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

Move CanIsolate CompletableFuture instance to 2.12+ only #985

Merged
merged 3 commits into from
Aug 24, 2019

Conversation

Avasil
Copy link
Collaborator

@Avasil Avasil commented Aug 22, 2019

There might be also a flaky CompletableFuture test, I'll fix it as well

@oleg-py
Copy link
Collaborator

oleg-py commented Aug 22, 2019

LGTM 👍

@Avasil
Copy link
Collaborator Author

Avasil commented Aug 22, 2019

I think CompletableFutureLocalSuite is failing because sometimes f doesn't get isolated reference before it is scheduled.

I will try to figure it out tomorrow but I am not familiar with CompletableFuture and why would it happen so if you guys have any ideas then go ahead :D

@oleg-py
Copy link
Collaborator

oleg-py commented Aug 23, 2019

I vaguely remember that pool management in CompletableFuture is worse than it is in our Scala one, and it often falls back to its default global pool. Maybe we can't really support it, then.

@Avasil Avasil mentioned this pull request Aug 23, 2019
@Avasil
Copy link
Collaborator Author

Avasil commented Aug 23, 2019

@oleg-py @alexandru

I vaguely remember that pool management in CompletableFuture is worse than it is in our Scala one, and it often falls back to its default global pool. Maybe we can't really support it, then.

Yup, I have been playing around and if we use thenCompose (equivalent of flatMap) it pretty much always uses ForkJoinPool.commonPool-worker-0 . Maybe it is still possible to support it but I don't think there is a lot of interest for it and it doesn't seem worth the effort right now. What do you think about removing the support in this PR? We can always revisit it later.

@oleg-py
Copy link
Collaborator

oleg-py commented Aug 23, 2019

Yeah, let's do that.

@alexandru
Copy link
Member

I vaguely remember that pool management in CompletableFuture is worse than it is in our Scala one, and it often falls back to its default global pool. Maybe we can't really support it, then.

Wait, what do you mean?

Is it our fault? Are we using global somewhere where we shouldn't?

@oleg-py
Copy link
Collaborator

oleg-py commented Aug 23, 2019

Is it our fault?

No, Java 8 CompletableFuture uses it's own "global" (ForkJoinPool.commonPool()), and it's hard-coded in methods since Java doesn't have implicits - unless you pass your own on almost every method call, it will reschedule on that commonPool

@Avasil
Copy link
Collaborator Author

Avasil commented Aug 23, 2019

Also note that thenComposeAsync doesn't have a variant which takes a thread pool (at least to my very limited knowledge)

@Avasil
Copy link
Collaborator Author

Avasil commented Aug 23, 2019

I pushed a change to remove it so we can easily merge it if we decide to go this way

@oleg-py
Copy link
Collaborator

oleg-py commented Aug 23, 2019

Yeah, I don't think we can or even should support anything that bypasses TracingScheduler.

@alexandru
Copy link
Member

👍 yes, screw it.

@Avasil Avasil merged commit 93eb2b5 into monix:master Aug 24, 2019
@Avasil Avasil deleted the bindLocal-completableFuture-2.12 branch December 14, 2019 13:25
mdedetrich pushed a commit to mdedetrich/monix that referenced this pull request Mar 28, 2020
* Move CanIsolate CompletableFuture instance to 2.12+ only

* Use handle instead of handleAsync in CanIsolate for CompletableFuture

* Remove CompletableFuture instance
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.

3 participants