Switch back cohttp-async expert response to use reader#842
Merged
rgrinberg merged 1 commit intomirage:masterfrom Jan 16, 2022
anuragsoni:use-reader-for-expert-response-async
Merged
Switch back cohttp-async expert response to use reader#842rgrinberg merged 1 commit intomirage:masterfrom anuragsoni:use-reader-for-expert-response-async
rgrinberg merged 1 commit intomirage:masterfrom
anuragsoni:use-reader-for-expert-response-async
Conversation
rgrinberg
approved these changes
Jan 16, 2022
Member
rgrinberg
left a comment
There was a problem hiding this comment.
Looks good. Will merge once CI approves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows us to remove Input_channel from the public api altogether. One interesting change here is the change in behavior in the server module where we no longer attempt to continue the server loop after the user provided
Experthandler returns. I think this behavior is okay for use-cases where the expert handler is used for connection upgrades (web sockets etc), and I am not sure if there are other common uses of the expert response_action outside of connection upgrades.With this change I think we'll maintain compatibility with any existing cohttp-async consumer that was using the expert response (ex: for websockets), so I think the additional complication of performing the conversion to a reader, and the switch in behavior of not attempting to continue the server loop after the expert handler returns is worth it