Fix cancelation leak in fromFuture, fromFutureCancelable
#3892
Conversation
| async[A](cb => as(delay(fut.onComplete(t => cb(t.toEither))), Some(fin))) | ||
| } | ||
| async[A] { cb => | ||
| flatMap(futCancel) { |
There was a problem hiding this comment.
I think we have to use uncancelable+poll. Otherwise starting the Future with futCancel is not cancelable, and it can be. It's only after we've started it, that we need guarantee the async is setup appropriately.
There was a problem hiding this comment.
I hope that is sufficient anyway, otherwise we'd have to drop down to cont 😬
There was a problem hiding this comment.
Well I did only say that I'd fix the leak 😛
Yeah I was wondering about that as I wrote it. I'll give it a go with uncancelable/poll. Although I did notice that in AsyncPlatform#fromCompletableFuture we changed it to use cont. I don't suppose you remember why?
There was a problem hiding this comment.
I don't suppose you remember why?
That's because the cancelation protocol for CompletableFuture is more complex: even if you request cancelation, it may indicate to you that it hasn't canceled in which case you need to fallback to waiting for the result. The best way to express this is with cont.
(Aside, but CE doesn't really handle this well, we still have leaks even in the cont-based implementation #3474).
leaks in fromFuture
|
@armanbilge I've updated now. It looks a bit weird to me but I think this has the correct behaviour? The construction of the future should not prohibit cancelation (by being I'm not sure about |
We can add some tests, that will help us understand if we got it right :) |
pfft I don't write bugs 😛 Yeah I'll do that. What do you think about the semantics of |
Absolutely never leak, |
|
Sorry @armanbilge I realized I wasn't sure where I should put these tests. They don't really feel like law tests but I can't find any other existing |
I guess that's fine too. It's all kind of a mess, in theory there is a distinction between tests of the |
Oh yeah |
fromCompletableFuture tests
| "fromFuture" should { | ||
| "backpressure on cancelation" in real { | ||
| // a non-cancelable, never-completing Future | ||
| def mkf() = Promise[Unit]().future |
There was a problem hiding this comment.
| def mkf() = Promise[Unit]().future | |
| def mkf() = Future.never |
Question: is this equivalent to making the whole thing cancelable (bad)
Thanks Daniel! :) |
armanbilge
changed the title
fromFuture, fromFutureCancelable
Fix leak in
fromFutureCancelableandfromFuture. Addresses #3891