Use Bracket in the Http1Writer.write

[danicheg]

A small enhancement. Semantics seems preserved.

[danicheg]

In case of merging this, I'll port to the 0.23 as well (because of Bracket to MonadCancel rebranding).

[rossabaker]

Did we lose raising the error here?

[danicheg]

guaranteeCase will pass further the raised error from the computing of FutureNoShift(F.delay(writeHeaders(headerWriter))) with adding finally-computation body.drain.compile.drain. If I'm correctly get it.

[RafalSumislawski]

So what this effectively changes is that on cancellation we will drain the body. Are you sure that this is what we want?

[danicheg]

@RafalSumislawski could you please elaborate on the cases you are worrying about. I can't imagine cases of user cancellation of FutureNoShift(F.delay(writeHeaders(headerWriter))). In case of cancellation of that computation caused by JVM kill, it doesn't matter what we'll do in that scenario. Or even if the connection will be killed. But yeah, maybe I'm missing some obvious things, though.

[RafalSumislawski]

You're right. At the moment (current series/0.22) nothing cancels Http1Writer.write, so it doesn't matter. IMO something should cancel it. (You can see my swing at that topic here https://github.com/http4s/http4s/pull/5385/files#diff-fab30518e463e4e678d5a1e12ff3a7ddc2ece7ef0a8417aef5332177a7838646R234 )

I'm not saying that we should deal with this in this PR, but just to explain myself:
I may be a bit paranoid when it comes to cancellation, because I keep chasing leaks and deadlocks in blaze caused by cancellation. I see that we need to drain the body because fs2 Streams are a bit like Resources and there's a risk that someone gave us a Stream which, when drained, is meant to release some preallocated resource (#1350 (comment)). The thing that I'm worried about is that we can't know how long body.drain.compile.drain will take. We can't even tell if it will ever complete. It becomes especially worrying when you want to cancel something (possibly because it's already running too long) and then cancelling it takes a long time (possibly due to the same cause which slowed down the first thing).

[rossabaker]

Are the problems that @RafalSumislawski is describing solved by the proposal of making a response a resource? Doesn't help for 0.22 or 0.23, of course.

[armanbilge]

I was reminded about typelevel/cats-effect#1979 about the cancelation model in CE3 which culminated in:

• Implemented timeoutAndAbandon for leaky timeout scenarios typelevel/cats-effect#2246

[RafalSumislawski]

@rossabaker It provides a better alternative to using the Stream's finalisation, so yes, in a sense it does solve the issue.

@armanbilge I'm aware of this discussion. I ok with waiting for the deallocation of the resources, what I'm worried about is the streamy nature of the stream. Let's say we cancel sending a request/response which happens to be streamed from a 100GB file. Waiting for the file descriptor to be released sounds fine. Reading and discarding the 100GB sounds bad.

[armanbilge]

@RafalSumislawski IIUC, on 0.22/0.23, the deallocation of resources is conflated with draining of the stream, which is something you might not want to wait for (or ideally do at all). Since we can only fix this with a Resource in 1.0/main, I was wondering if timoutAndAbandon could improve the situation for 0.22/0.23. But maybe blocking on the drain is the lesser of two evils.

[RafalSumislawski]

@armanbilge
timeoutAndAbandon may create it's own set of problems. I'm wondering if we could do something along the lines of starting the drain and then cancelling/interrupting it in order to trigger the deallocation without waiting for the drain. I will have to experiment with it. Ideally I would like to have a test exercising the cancellation of a "long" Stream with an embedded resource, but as @danicheg pointed out, the Http1Writer.write simply isn't cancelled in the current implementation. That's why I'm ok with merging this PR as is, and dealing with the discussed problem independently.