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

Introduce OutBodyStreamingUnmask #80

Merged

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Jul 24, 2023

This is a new attempt at providing an unmask callback to streaming requests, replacing #77.

As per the discussion in #77, I've new introduced OutBodyStreamingUnmask alongside OutBodyStreaming.

This was actually a useful change. When I first wrote #77, I didn't really understand the server part of http2 very well yet, as I had not yet implemented the server part of my gRPC library. I now have, and understand the server aspect of http2 much better. My suspicion in #77 that I might have gotten the control flow wrong for the server API turned out to be correct: the point of this change is that user code gets control over when exceptions are unmasked in newly spawned threads. On the client side, that newly spawned thread is created in sendRequest (in Network.HTTP2.Client.Run). On the server-side, however, this works a bit differently: http2 spawns a new thread for each incoming request (call to setAction in run, Network.HTTP2.Server.Run); it then does not spawn a new thread for a streaming response body.

Therefore, this PR only deals with the client side, and OutBodyStreamingUnmask is not actually supported at all on the server side (this PR introduces requestStreamingUnmask, without a corresponding responseStreamingUnmask). It might be useful to change the Server type synonym so that the server action passed to run would also be given an unmask callback, but that has now become orthogonal to this PR.

This is only supported in the client, not in the server, for reasons explained
in the code.
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.

Beautiful!

@kazu-yamamoto kazu-yamamoto merged commit 930d905 into kazu-yamamoto:master Jul 25, 2023
10 of 15 checks passed
@kazu-yamamoto
Copy link
Owner

Merged.
Thank you!

@edsko edsko deleted the edsko/outbodystreamingunmask branch July 25, 2023 06:42
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.

None yet

2 participants