Well, it happens slowly (I guess), so i spawn four of those loops in parallel (then it happens quickly). See: https://gist.github.com/TomasStanek/f06b89d888e0aed2350375470367cbcd
I'm not saying we don't have a bug, I am saying that the currently released version shouldn't leak.
by "not being synchronized" I mean that the action which actually reacts to cancelation (like removing the dequer/peeker in fs2.async.mutable.Queue, or removing timeout from some timeoutScheduler) may happen later than println is called in queue.peek1.start.flatMap(fib => fib.cancel).flatMap(_ => println("not yet canceled"); ...)
I will open an issue with more detailed description later today. I'm new to cats/cats-effect so I wanted to ask first if I'm not doing something silly.
Most likely, the loosing peeker is removed after another one is registered. (There may be only one peeker in the Queue implementation)
@TomasStanek the current code is like this:
if (isActive.getAndSet(false)) {
// First interrupts the other task
try other.cancel() finally {
main.pop()
cb(Right(r))
}What happens is that the loser gets cancelled before the winner's result is signaled. Now if that cancellation logic is synchronous, there shouln't be any issues. But if that cancellation logic happens asynchronously, then indeed you might have issues, however cancelling a timeout shouldn't be async obviously.
In the new code in that PR, it is now changed to this:
if (isActive.getAndSet(false)) {
// First interrupts the other task
other.cancel.unsafeRunAsync { r2 =>
main.pop()
cb(Right(r))
maybeReport(r2)
}So now the cancellation can happen asynchronously, although there are some caveats here as well, because due to multi-threading, there's always the possibility that the finalizer you want might not be registered yet, but that's another discussion.
So for simple things, like operating data structures or cancelling a timeout, the current logic should work. We might have a leak, there's always that possibility, but we'll need to do some profiling because it isn't obvious at all.
@alexandru I've tried compiling against branch pr/305 (commit: bbb15854098d5eb3887a10702de5fe41fb886911) and the leak is still there (there are java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask stacking up and java.util.concurrent.RunnableScheduledFuture[] array keeps growing).
The issue with queue.peek1 deadlock also persisted.
I'll open a proper issue hopefully tomorrow evening
The issue with queue.peek1 deadlock also persisted.
I'll open a proper issue hopefully tomorrow evening
I'm looking at
Resource and struggling to understand why it's there; it appears superfluous to Bracket, as Resource.use requires an implicit Bracket. Is this just so that if your clean-up code is unwieldy, you can avoid writing F.bracket(<my huge initialization/finalization code>)?
forget about Resource for a second
look at this code
for now I'll be talking about fs2 0.10, we changed the signature to make things clearer in 1.0, which will hopefully be clear soon
I did notice that you changed e.g.
io.Socket into a Resource. I guess that' swhere this is going ... but go on.
how do you think each will behave?
The second bit of code looks dodgy, because of the
pure.
with the Stream version, you can just emit the resource, and it will all be fine (emit is also
pure)
with the F version, by the time you flatMap the file is closed, and
readFromFile fails
if you want to understand this inconsistency, this is the mental model
bracket closes a resource as soon as the F finishes emitting elements
now, IO can emit one element at most
so when you do
pure, you emit, so you have finished emitting, so the resource gets closed
when you do
emit on Stream, the stream has not finished emitting, because a Stream, unlike IO, can emit multiple elements, and therefore the thing is still fine
This explains a problem I've been having for a few weeks...
Whereas the F version releases as soon as the first (and only) element is emitted.
so when you do
stream1 ++ stream2
it will release things at the end of
stream1
you got the F version right
now
(I'm assuming that the flatMap prevents it from releasing the resources at the end of the first stream, right?)
My point here is that the
sleep (or delay or whatever) doesn't produce any values, it is only evaluated for effects.