Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Howto cancel async retries #52

Closed
leozilla opened this issue Sep 5, 2016 · 7 comments
Closed

Howto cancel async retries #52

leozilla opened this issue Sep 5, 2016 · 7 comments
Labels

Comments

@leozilla
Copy link

leozilla commented Sep 5, 2016

Can you add a short example howto cancel async retries.

I was surprised that canceling the returned CompletableFuture does not cancel the retries.
Am I using the library wrong or is this intended?
How do I cancel the retry in the following example, do I need to add a canceled flag and check this in the future method?

scheduler = Executors.newScheduledThreadPool(1);
executor = Executors.newCachedThreadPool();

CompletableFuture<Proxy> proxyFuture = Failsafe.with(proxyRetryPolicy)
                       .with(scheduler)
                       .future(asyncExecution -> {
                           return CompletableFuture.supplyAsync(proxySupplier, executor);
                       });

// does not cancel the retry .. keeps retrying
proxyFuture.cancel(false);
proxyFuture.cancel(true);
@jhalterman jhalterman added the bug label Sep 6, 2016
jhalterman added a commit that referenced this issue Sep 6, 2016
@jhalterman
Copy link
Member

@leozilla Just pushed a fix for this. This issue has to do with performing an execution on via .future specifically where the resulting CompletableFuture cancellation was not propagated to the associated FailsafeFuture which would stop continued execution. Maybe you can give this a try and let me know if it resolves your problem. I pushed a test case here:

https://github.com/jhalterman/failsafe/blob/732746b013bb93f3d3d29ac3f026af19ef3cc532/src/test/java/net/jodah/failsafe/issues/Issue52.java#L36-L59

@leozilla
Copy link
Author

leozilla commented Sep 7, 2016

Ah, thanks!
I did not expect this to be a bug otherwise I would have tried to fix it myself.
Anyway I try your fix now and let you know how it worked out.

@jhalterman
Copy link
Member

You should be able to just pull master and mvn package to give it a try. Tnx.

@jhalterman
Copy link
Member

jhalterman commented Sep 8, 2016

Going to close this now ahead of the next release. @leozilla if the fix ends up not looking good on your end, feel free to re-open.

@jhalterman
Copy link
Member

Released 0.9.3 with this fix.

@leozilla
Copy link
Author

leozilla commented Sep 9, 2016

Sorry for the delay, it works for me.
Just one more minor thing.
The test ExecutionTest#shouldSupportMaxDuration() breaks randomly on windows with oracle jdk 1.8.0_91
Reason seems to be the Thread.sleep is exactly as long as the max duration.

Fails

public void shouldSupportMaxDuration() throws Exception {
    Execution exec = new Execution(new RetryPolicy().withMaxDuration(100, TimeUnit.MILLISECONDS));
    assertTrue(exec.canRetryOn(e));
    assertTrue(exec.canRetryOn(e));
    Thread.sleep(100); // <--
    assertFalse(exec.canRetryOn(e));
    assertTrue(exec.isComplete());
  }

Succeeds (although still a little ugly)

public void shouldSupportMaxDuration() throws Exception {
    Execution exec = new Execution(new RetryPolicy().withMaxDuration(100, TimeUnit.MILLISECONDS));
    assertTrue(exec.canRetryOn(e));
    assertTrue(exec.canRetryOn(e));
    Thread.sleep(105); // <--
    assertFalse(exec.canRetryOn(e));
    assertTrue(exec.isComplete());
  }

Should I provide a fix/PR for it?

@jhalterman
Copy link
Member

Yea, that would be a good change. PR welcome :)

fzakaria pushed a commit to fzakaria/failsafe that referenced this issue Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants