Skip to content

Fix CompletableFuture cancelation#2904

Closed
bplommer wants to merge 1 commit intotypelevel:series/3.xfrom
bplommer:async-from-completable-future
Closed

Fix CompletableFuture cancelation#2904
bplommer wants to merge 1 commit intotypelevel:series/3.xfrom
bplommer:async-from-completable-future

Conversation

@bplommer
Copy link
Copy Markdown
Contributor

I think this fixes #2903 but I haven't yet written a regression test.

@bplommer
Copy link
Copy Markdown
Contributor Author

cc @Daenyth

@vasilmkd
Copy link
Copy Markdown
Member

LGTM, but a test would be really good for this.

@armanbilge
Copy link
Copy Markdown
Member

We discussed how to write a test on Discord.
https://discord.com/channels/632277896739946517/632278585700384799/956566207211905115

The tl;dr is create an uncancelable "never" CompletableFuture that will not complete, and always returns false when attempts to be canceled. The correct implementation should get backpressured on cancellation, whereas the broken one should terminate.

@bplommer
Copy link
Copy Markdown
Contributor Author

bplommer commented Mar 24, 2022

There was some discussion on Discord about how to test this - I can't promise to do that in the next couple of days, so feel free to fork this PR with a test.

Edit: what @armanbilge said

@vasilmkd
Copy link
Copy Markdown
Member

@armanbilge Can you please handle this? Thanks.

@armanbilge
Copy link
Copy Markdown
Member

Yup I'm working on #2902, I can do this next.

@vasilmkd
Copy link
Copy Markdown
Member

Closing in favor of #2907.

@vasilmkd vasilmkd closed this Mar 24, 2022
@bplommer bplommer deleted the async-from-completable-future branch March 24, 2022 21:49
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.

Async.fromCompletableFuture should block if cancelation fails

3 participants