-
Notifications
You must be signed in to change notification settings - Fork 591
Refactor HttpHandlers to use fewer arguments and to stream responses #10491
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
Closed
Conversation
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
20f44e0
to
9d518af
Compare
Al2Klimov
reviewed
Jun 30, 2025
b01c538
to
04dc8f2
Compare
Al2Klimov
reviewed
Jul 1, 2025
24d6d3e
to
cb42cdd
Compare
7a55570
to
d64106c
Compare
c3bc75b
to
46951a9
Compare
d64106c
to
883ea0e
Compare
46951a9
to
43a9980
Compare
883ea0e
to
3098345
Compare
43a9980
to
251e43a
Compare
0d7b984
to
fa99c02
Compare
julianbrost
reviewed
Jul 2, 2025
Al2Klimov
reviewed
Jul 2, 2025
b52e873
to
ace70d8
Compare
a1dc496
to
5d731a3
Compare
efd0d0b
to
10ac1b0
Compare
Al2Klimov
reviewed
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to #10491 (review):
0fdced2
to
3c260a5
Compare
3c260a5
to
151d8a8
Compare
These parameters are no longer needed since they were only used by EventsHandler which was refactored in an earlier commit.
151d8a8
to
a0d56ed
Compare
I'm addressing the remaining comments and then I'm going to close this draft PR and open an new one for proper review, mostly so we don't have to scroll past 100 force pushes and resolved conversions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR is a work in progress. It fixes #10142 and is a step towards #10409.
This is based on the improvements to @yhabteab's improvements to the JSON encoder in #10414.
Description
HttpRequest
that incorporates all of the derived data, like the authenticated user, url and parameters, that the handlers need, so the don't have to be passed as individual arguments, as per the suggestion in Simplify HttpHandler::HandleRequest() signature by grouping arguments into structs #10142.HttpResponse
, supported by a new custom message body type that makes it easy to use theboost::beast::http::serializer
to start sending some responses with chunked encoding, before the complete data andcontent_length
information is available.boost::beast:message
types and are usable in functions that require these types.WaitGroup
, the request and response objects and theyield_context
being passed to the handlers.EventsHandler
but without adding a solution specific to it (like StartStreaming did previously), I've added a third coroutine toHttpServerConnection
that waits for the stream to be shut down by the remote side and then initiates aDisconnect()
.HttpServerConnection
,HttpRequest
andHttpResponse
. These add lots of functionality that can be used to extend unit-tests to other components (such as the individual HTTP handlers, JsonRpcConnection, etc.) in the future.Todo
TheEventsHandler
currently still blocks formally disconnecting the connection until the next event is returned by theEventsInbox
. Before this PR, that was handled inHttpServerConnection::StartStreaming()
by trying to read from the connection in another coroutine and callingHttpServerConnection::Disconnect()
when the connection is closed. The handler was still sitting there, but it could simply return when it got the next event while the connection was already disconnected.The signalling between the coroutines needs some more work. Possibly this can be done more implicitly. Specifically I want to get rid ofHttpResponse::Begin()
andHttpResponse::Done()
.Get distros using boost-1.66 (opensuse/leap, rockylinux:8) to build. 1.66 was the first versionboost::beast
was introduced and there are some pesky differences to the interface of the message body implementation needs to satisfy.Look into adding test-cases for Http(Request|Response) and possibly HttpServerConnection (minimal version without handlers)