Skip to content
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

Semantically breaking change in SocketGroup.serverResource #2289

Open
RaasAhsan opened this issue Feb 20, 2021 · 1 comment · May be fixed by #2300
Open

Semantically breaking change in SocketGroup.serverResource #2289

RaasAhsan opened this issue Feb 20, 2021 · 1 comment · May be fixed by #2300
Labels
Milestone

Comments

@RaasAhsan
Copy link

On 2.5.x, the return type for serverResource is:

Resource[F, (InetSocketAddress, Stream[F, Resource[F, Socket[F]]])]

On the latest 3.x milestone, the return type is now:

Resource[F, (SocketAddress[IpAddress], Stream[F, Socket[F]])]

To preface, the change in the signature makes complete sense from a resource safety perspective. IIRC the former definition is actually a little bit misleading; the socket associated with the inner Resource has already been allocated before use is even called! As a consequence, if use is not called, the socket is never closed and gets stuck in limbo, resulting in a file leak.

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.

Ember Server exploited the lack of safety in the old definition to implement graceful shutdowns: we can interrupt the server stream and force closure upon it while allowing existing connections to complete their work for some time. parJoin semantics are actually too strict because inner streams lease the input stream's scope and prevents its resources from finalizing until all inner streams are closed. So we implemented a weaker version of parJoin called forking that doesn't lease any scopes, which is unsafe in general but completely fine for this particular use case.

forking completely breaks with the new definition of serverResource precisely because it doesn't lease the scope. Since the accepted sockets are bound to the server stream, as soon as a connection stream is spawned, the socket is going to finalize. Is there a way to only partially lease some resources from the server stream in a safe manner? In particular, lease the accepted socket but not the server socket.

This is blocking Http4s builds on CE3 ATM, and reverting to parJoin would be a pretty major regression in behavior (and performance to a lesser degree), so it would be cool if we could come to a resolution on this soon

@RaasAhsan RaasAhsan added the fs3 Issues related to the topic/fs3 branch label Feb 20, 2021
@RaasAhsan
Copy link
Author

RaasAhsan commented Feb 22, 2021

The larger issue here is that the lifecycles of the server socket and all accepted sockets are now bound to the same stream.

Ok so this is actually not true, as evidenced by the signature. The server resource is the outer resource and the client sockets are on the inner stream. However, Ember Server (and others I imagine) forcibly bind it to the same stream via Stream.resource(...).flatMap(...). I'm going to see if untangling that works, but I have a feeling that it won't, in which case I have another idea to make the old Resource signature safe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants