-
Notifications
You must be signed in to change notification settings - Fork 788
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
Merge 0.22 -> 0.23 #6125
Merge 0.22 -> 0.23 #6125
Conversation
Flush the prelude in non-blocking Servlet IO
…ually Render continually between response prelude and body
Release v0.21.32
Build tweaks in case there's another 0.21
This reverts commit 229e120.
Exclude series/0.21 from release notes
Simplify website configuration
// Binary compatibility hack | ||
F match { | ||
case ce: ConcurrentEffect[F] => | ||
Http4sServlet.renderResponseContinually(response, servletResponse, bodyWriter)(ce) | ||
case _ => renderResponseDiscontinually(response, servletResponse, bodyWriter) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh.
Also we should merge #6129 first. |
Add unidocs project to root aggregate
// This F.attempt.flatMap can be interrupted, which prevents the body from | ||
// running, which prevents the response from finalizing. Woe betide you if | ||
// your effect isn't Concurrent. | ||
F.uncancelable { poll => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a crude port of the F.continual
, but I think not exactly what we want. We don't want to make bodyWriter
or response.body.drain
uncancelable. We want to ensure that the response.body finalizer runs if we acquired a response. This ugliness is what we eventually hope to resolve by making responses resourceful.
This needs to be the same logic in blaze and ember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can poll
on on the bodyWriter
and the left expression, too. But that's still not exactly correct: we need to finalize the stream, but the finalizer only runs if the stream compilation gets a chance to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can make this program complete and print true
, we can plug the leak.
Ref[IO].of(false).flatMap { ref =>
val s = Stream.never.onFinalize(ref.set(true))
IO.uncancelable { poll =>
IO.canceled *> ???
}.guarantee(ref.get.flatMap(IO.println(_)))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need poll
around bodyWriter(response)
and response.body.drain....
. I also think you probably want to toss an onCancel
on your poll
s.
we need to finalize the stream, but the finalizer only runs if the stream compilation gets a chance to start
Why can't you rely on onCancel
on the poll
wrapping the stream compilation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, poll
ing bodyWriter
and response.body.drain
seems necessary. But what would onCancel
do? response.body.drain
perhaps, but what if bodyWriter
got interrupted partway through, and response.body
's effects are not idempotent?
Again, this is all caused by an unfortunate design choice from before we had Resource
. But it's what we have in 0.23, and it affects every backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also why we can't do something like:
Stream.bracket(acquireResponse)(_.body.drain.compile).flatMap(renderResponse)
We can make this as leakproof as 0.22 is and move on, but since we hit merge conflicts, it does seem like a nice opportunity to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is challenging because there isn't a strict separation of acquire-emit-release. There are nested scopes. We might "finalize a response" by calling onFinalize
on the body, and then in a middleware wrap more effects. The only way to release all resources acquired for the response but finalized in its body is to uncancelably drain the entire body. Gross!
The right solution is still a resource of a response. Something that could work in 0.23 would be a vault attribute that contains a finalizer. Then a response would look much like a Resource.Allocated
and be treated as such. But it would be contrary to how we've told people to finalize resources since dinosaurs roamed the earth.
In any case, I don't think we can solve it the way I envisioned, nor should solve it in this merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot to think about. But yeah, would be good to get this merge done, fix the website, and unblock the release train :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am intentionally not merging from 0.22. There'll be another train soon enough.
I'll do the merge into main, in a little bit. |
…-20220313 Merge 0.22 -> 0.23
…-20220313 Merge 0.22 -> 0.23
Check my work in the non-ember backends since I don't follow the action there too closely.