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

Close all streams on termination #83

Closed

Conversation

edsko
Copy link
Collaborator

@edsko edsko commented Aug 25, 2023

This is the second patch in a patch set of three patches, the first being #82 (the first commit in this PR is #82).

When a client disconnects, http2 kills the corresponding handlers. This may be fine for a webserver where the handler is just feeding the client information, but it's not fine in the general case. For example, I am building a gRPC library; when a client makes a remote procedure call, the server handler must have the option to terminate cleanly; after all, an RPC could be anything, and the server might need to execute some code after the client has told it all it needs to; maybe it needs to write to a database, maybe it needs to make RPC calls of its own, maybe a lot of things; simply killing the handler is not ok.

In my library I am isolating the handler from the KilledByHttp2ThreadPoolManager exception by running it in a separate thread; when the parent thread receives the KilledByHttp2ThreadPoolManager exception, it marks some local state to indicate that the handler is no longer able to send anything to the client. If the handler attempts to do so at this point, an exception will be raised, but otherwise the handler is allowed to continue.

On the input side things are more complicated, however, which is where this PR comes in. While it is possible to have some local state indicating that the handler shouldn't expect any more input from the client either, there might still be unprocessed input from the client in the TQueue. We don't get that direct access to that TQueue in the http2 API; all we have is getRequestBodyChunk; this makes it difficult to write code that says "check this local piece of state but only if the TQueue is exhausted".

Prior to this PR, if we call getRequestBodyChunk when the TQueue is empty, we get an exception, but not a useful one ("thread blocked indefinitely"). This PR changes this by making two changes:

  1. The TQueue can carry an exception; when we encounter this exception, it will be raised in getRequestBodyChunk.
  2. When we terminate, we close all still open streams; if we terminated normally (that is, ConnectionIsClosed) we write an empty bytestring to the queue, to allow handlers to detect normal client termination; if we terminate with any other exception, we write that exception to the queue, allowing the handler to detect that the client terminated with an exception.

@edsko edsko mentioned this pull request Aug 25, 2023
Importantly, this means we properly close all streams when a client
disconnects.
@edsko edsko force-pushed the edsko/close-streams-on-termination branch from a92a5aa to 17cc0da Compare August 25, 2023 14:01
@kazu-yamamoto kazu-yamamoto self-requested a review August 28, 2023 01:05
Copy link
Owner

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kazu-yamamoto added a commit that referenced this pull request Aug 28, 2023
@kazu-yamamoto
Copy link
Owner

Rebased and merged.

@edsko edsko deleted the edsko/close-streams-on-termination branch August 29, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants