typelevel
Shared server sockets
Intends to fix #2289. Relevant Gitter conversation located here: https://gitter.im/functional-streams-for-scala/fs2?at=6035dec142f30f75c7c35746
So, the idea here is that the lifecycles of server sockets and client sockets have independent lifecycles, so it should be possible for us to finalize them separately. This was possible to do in 2.5.x because the SocketGroup server signature returned a Stream[F, Resource[F, Socket[F]]], in conjunction with the forking combinator that was developed in Ember.
Unfortunately, that method is leaky (what happens if you don't call use on the inner resource?), so the signature changed to Stream[F, Socket[F]], where the sockets are actually bound to the server stream. parJoin is necessary here in order to lease the scope and prevent the socket from immediately finalizing, but this sneakily also leases the server socket, preventing that from finalizing until all client sockets have finalized!
One solution to maintaining independent resource lifecycles while preserving safety is to implement a data structure called Shared that can facilitate the transfer of ownership of resources, which is captured in this PR. A client socket can be allocated on the server stream into a shared resource and handed off to the client stream which will acquire the resource. After that, the server socket can release the resource before moving onto accepting the next socket (this won't actually happen when using parJoin because of leasing).
I think the forking function is going to have change a bit; it'll have to operate on a Stream[F, Stream[F, Shared[F, O]]] because we need to ensure that the shared socket is acquired before control is returned to the accept stream. If it doesn't, the accepted socket is going to be immediately finalized after return. See: https://github.com/http4s/http4s/blob/main/ember-server/src/main/scala/org/http4s/ember/server/internal/StreamForking.scala#L71 . In comparison to parJoin, the new definition of forking replaces the notion of implicit scope leasing with the notion of explicit resource transfer.
Notes:
- The current
serversignatures are completely safe when used withparJoin, but I think we can even just revert them to their previous versions, and add a separate signature that exposesShared. - If this goes through, I'm wondering if we should try to include a (safer) variant of
forkingin fs2 that people can use to exercise the graceful shutdown behavior - Need to look at fs2-netty to see how this plays out there
- Is it possible to formalize safe, implicit/explicit resource transfer into
Streamsomehow? Keyword there is safe
Sorry for dropping the ball on this (I've been a bit busy), but I think it's ready for a deeper review. It does deserve some tests which I'll try to get to ASAP, but I'm curious to hear thoughts on general approach. I wonder if it's worth exploring a more Stream-native mechanism for resource ownership transfer in the future.
@mpilquist @RaasAhsan I was actually looking at the StreamForking code yesterday and it seems like it would be a big win. Would you be interested in me trying to get this over the line and if so, is there agreement on what is required? Thanks! :)
@TimWSpence I need to take a deeper look at this PR, but you may be interested in the change I just made in https://github.com/typelevel/fs2/pull/3063/commits/9e6a59b8490689a2301d843c9587a5d82e6fbdb5.
In fact I have some crazy idea that it fixed this :P but yeah, let me actually read about the issue first 😂
@TimWSpence I need to take a deeper look at this PR, but you may be interested in the change I just made in 9e6a59b.
In fact I have some crazy idea that it fixed this :P but yeah, let me actually read about the issue first 😂
Oh interesting! Is there a parent issue or PR for some conext on that change?
@armanbilge thinking about it a bit more, a reference-counted resource wrapper like this might be quite useful generally in cats effect. What do you think?
@armanbilge I don't think your change does fix this - we still have https://github.com/typelevel/fs2/pull/3063/files#diff-f11c9120aaf385f6214d8d1a82560c137b058a244c84ad7d8b7126e78b2081aeL128 which means that the lifecycle of the connection is tied to that of the outer server stream
@TimWSpence perhaps I misunderstood the original problem. I'm going on https://github.com/typelevel/fs2/issues/2289:
The larger issue here is that the lifecycles of the server socket and all accepted sockets are now bound to the same stream. However, in practice, it's completely sound for them to have disjoint lifecycles e.g. you can close a server socket while leaving sockets that were accepted open.
I'm pretty sure the changes I made make those lifecycles disjoint: the server now accepts sockets into a Channel, which is consumed as a stream. Furthermore, the connection lifecycles are tied to the consuming stream, not the stream producing accepted sockets into the Channel. So it should be fully possible to close the server socket and thus close the Channel, without terminating the consuming stream.
@TimWSpence perhaps I misunderstood the original problem. I'm going on #2289:
The larger issue here is that the lifecycles of the server socket and all accepted sockets are now bound to the same stream. However, in practice, it's completely sound for them to have disjoint lifecycles e.g. you can close a server socket while leaving sockets that were accepted open.
I'm pretty sure the changes I made make those lifecycles disjoint: the server now accepts sockets into a
Channel, which is consumed as a stream. Furthermore, the connection lifecycles are tied to the consuming stream, not the stream producing accepted sockets into theChannel. So it should be fully possible to close the server socket and thus close theChannel, without terminating the consuming stream.
Oh sorry, yeah I missed that those resources are tied to the stream coming from the channel. From some brief experimentation it didn't look like it was working for me though. I'll do some more digging
Well, maybe that's not really the problem 😂 see subsequent comment in https://github.com/typelevel/fs2/issues/2289#issuecomment-783510663. I'm not sure if FS2 actually needs any changes. I suspect we can do this http4s, maybe with some strategic use of allocatedCase to manage the outer resource.
Well, maybe that's not really the problem 😂 see subsequent comment in #2289 (comment). I'm not sure if FS2 actually needs any changes. I suspect we can do this http4s, maybe with some strategic use of
allocatedCaseto manage the outer resource.
🤦 how did I miss that. Nice, I'll poke at http4s a bit and see what I can come up with