Skip to content

Async EDT.schedule#216

Open
aprilfire wants to merge 2 commits intomasterfrom
DL-5027
Open

Async EDT.schedule#216
aprilfire wants to merge 2 commits intomasterfrom
DL-5027

Conversation

@aprilfire
Copy link
Contributor

Make EDT accept suppliers and return asyncs

@aprilfire aprilfire requested a review from glebleonov November 22, 2017 12:51
public void schedule(Runnable runnable) {
myManager.getEdt().schedule(runnable);
public Async<Void> schedule(Runnable runnable) {
return myManager.getEdt().schedule(runnable);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand kill() contract, it should fail all asyncs that was scheduled in corresponding edt. As far as I see, this implementation fails to follow such contract.


long getCurrentTimeMillis();
void schedule(Runnable r) throws EdtException;
Async<Void> schedule(Runnable r) throws EdtException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be we need to clarify threading of callbacks and kill/finish semantics?

@Override
public void run() {
task.run();
myUnresolvedAsyncs.remove(i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can shutdownNow() interrupt this runnable between task.run() and myUnresolvedAsyncs().remove? If so, kill() method can throw exception about failing already completed async

Async<Void> schedule(Runnable r) throws EdtException;
<ResultT> Async<ResultT> schedule(Supplier<ResultT> s) throws EdtException;
<ResultT> Async<ResultT> flatSchedule(Supplier<Async<ResultT>> s) throws EdtException;
Registration schedule(int delay, Runnable r) throws EdtException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be schedule(int delay, Runnable r) should return AsyncWithRegistration?

}


private Registration schedule(int delay, RunnableWithAsync<?> r) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a lot of duplication in two schedule() methods

public void run() {
try {
T resultT = supplier.get();
if (fulfilled.compareAndSet(false, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any cases when this compareAndSet will happen?

return async(s);
}

private static <T> Runnable fromPlainSupplier(final Supplier<T> supplier, final ThreadSafeAsync<T> async,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is duplication in fromPlainSupplier and fromAsyncSupplier methods

scheduleInfiniteAndKill(getDefaultSupplier());
}

private void scheduleInfiniteAndKill(Supplier<?>... restTasks) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice test :)

void schedule(Runnable r) throws EdtException;
Async<Void> schedule(Runnable r) throws EdtException;
<ResultT> Async<ResultT> schedule(Supplier<ResultT> s) throws EdtException;
<ResultT> Async<ResultT> flatSchedule(Supplier<Async<ResultT>> s) throws EdtException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you illustrate how this method will be used?


final class EdtTestUtil {
private static final int VALUE = 42;
static Supplier<Integer> getDefaultSupplier() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a blank line after 42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants