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

Server cannot distinguish between clean and unclean client disconnect #209

Closed
edsko opened this issue Aug 1, 2024 · 7 comments
Closed
Labels
bug Something isn't working priority: medium Should be done before the library can be considered complete

Comments

@edsko
Copy link
Collaborator

edsko commented Aug 1, 2024

Currently, when a client terminates with an exception, the HTTP stream is closed normally, which on the server side, if the handler is currently to read, will result in the handler seeing NoMoreElems rather than ClientDisconnected.

gRPC does not allow for client trailers, so unsure if we can fix this; perhaps we can terminate the HTTP stream in a different manner using a low-level HTTP2 feature.

We should verify what happens if the client is, say, a Python or C++ client.

Not sure if this is a bug or not; marking it as a bug for now, because telling the handler that the client signalled NoMoreElems is wrong. However, not 100% sure if this bug is fixable (it might be a"bug in the gRPC spec" -- should have allowed for client trailers).

@edsko edsko added bug Something isn't working priority: medium Should be done before the library can be considered complete labels Aug 1, 2024
@edsko
Copy link
Collaborator Author

edsko commented Aug 1, 2024

A consequence of this problem is that even when the server tries to respond to the client (with trailers, say), it won't realize that the client has disappeared. After all, the client just indicated that they have no more data for the server, and the server doesn't realize something bad has happened; from the server's perspective, this looks like an entirely normal situation. This is even more reason to try and close the stream in a non-clean fashion.

@edsko
Copy link
Collaborator Author

edsko commented Aug 1, 2024

My suspicion (unverified):

  1. When we exit the scope of withRPC, we call Session.close.
  2. Session.close has this comment:
    -- We leave the inbound thread running. Although the channel is closed,
    -- there might still be unprocessed messages in the queue. The inbound
    -- thread will terminate once it reaches the end of the queue
  1. It's possible that the way that we use http2 (the client) in this manner leaves the HTTP2 stream in half closed mode: the client can no longer send to the server, but the server can still send to the client. The comment "The inbound thread will terminate once it reaches the end of the queue" here is confusing, but I'm not entirely sure how exactly we reach the end of the queue" -- maybe it just doesn't read the end of the queue until the server itself decides to close its connection!

If all this is correct:

  1. We need a way to cause http2-the-client to put the HTTP2 stream in fully closed state. (It may be sufficient to terminate the recv message loop.)
  2. This would in turn cause http2-the-server to send KilledByHttp2ThreadManager to the handler, which grapesy would catch and close the server-side channel -- so that if the server handler tries to send to the client, it will receive an exception (ClientDisconnected).
  3. But we left the inbound thread running for a reason (see Haskell comment above): we cannot kill the inbound thread in all situations where we call Session.close. However the case where we leave the scope of withRPC does seem to be a case where killing the inbound thread is okay: the user anyway has no way of accessing any remaining messages in the queue, since they do not have a Call object anymore.

@edsko
Copy link
Collaborator Author

edsko commented Aug 1, 2024

Note: for this to be a reliable, there should not be any race conditions. The server should not first see the outbound STREAM closing (resulting in grapesy reporting NoMoreElems), and only then see the stream completely closing (causing the handler to receive the killed exception).

@edsko
Copy link
Collaborator Author

edsko commented Aug 1, 2024

We should verify in Wireshark the theory that the stream is only half-closed when the client leave the scope of withRPC.

@edsko
Copy link
Collaborator Author

edsko commented Aug 1, 2024

Perhaps the cleanest way to solve this is to add an API to http2 to explicitly cancel a Request?

@edsko
Copy link
Collaborator Author

edsko commented Aug 29, 2024

@FinleyMcIlwaine Once kazu-yamamoto/http2#142 and kazu-yamamoto/http-semantics#11 are merged, we can resolve this issue right?

@edsko
Copy link
Collaborator Author

edsko commented Sep 5, 2024

Closed in #211.

@edsko edsko closed this as completed Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: medium Should be done before the library can be considered complete
Projects
None yet
Development

No branches or pull requests

1 participant