-
Notifications
You must be signed in to change notification settings - Fork 597
Refactor HttpMessage into generalized templated types #10692
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
Conversation
b306484 to
bcabeef
Compare
yhabteab
left a comment
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.
The implementation has been left in the source file so templating the classes doesn't blow up the compile times too much. This has the caveat that any new code that wants to use different instances of the template will have to add their instantiation to
httpmessage.cpp, but I think it's better this way.
What does that mean for users that for example don't want to use boost::beast::multi_buffer as a buffer type for the body? I don't like the fact that this buffer type can return multiple buffers on prepare calls, which makes it harder to work with in my PR with zero-copy serialization. The beast flat_buffer on the other hand, always returns a continues memory region represented by a single buffer on prepare calls. So, would users that want to use flat_buffer instead of multi_buffer have to add their own instantiation of the OutgoingHttpMessage template to this file? That seems a bit inconvenient!
Is there a better way? I'm not saying we should move the implementation to the header file, but maybe we can find a middle ground?
Yes, you then add another explicit instantiation to I'd add the definitions to the header, but I really think it might start impacting compile times as more and more other translation units include this header (either directly or transitively). And since we don't add new instances of this class all the time (like std::map or std::vector etc.) but probably just a couple if any in years to come, I think this is fine compromise.
🤦 I actually meant to use that in the generic |
This adds generalized IncomingHttpMessage and OutgoingHttpMessage templates that support different types of streams (via a std::variant) and can both be used for either requests or responses. The tacked on metadata from the old HttpRequest and server connection from the old HttpServerConnection have been moved to HttpApi(Request|Response) classes that derive from the above generalized message types.
bcabeef to
1505f09
Compare
yhabteab
left a comment
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.
Awesome 🎉!
curl -ksSiu root:icinga 'https://localhost:5665/v1/config/files/example-cmdb/3a8d9807-6869-4512-b2e6-8e96b81e19a6/startup.log?pretty=1'
HTTP/1.1 200 OK
Server: Icinga/v2.15.0-231-g1505f09ed
Content-Type: application/octet-stream
Content-Length: 1898
[2026-01-23 09:19:51 +0100] information/cli: Icinga application loader (version: v2.15.0-231-g1505f09ed; debug)
[2026-01-23 09:19:51 +0100] information/cli: Loading configuration file(s).
[2026-01-23 09:19:51 +0100] warning/config: Ignoring directory '/Users/yhabteab/Workspace/icinga2/prefix/etc/icinga2/zones.d/master' for unknown zone 'master'.
[2026-01-23 09:19:51 +0100] warning/config: Ignoring directory '/Users/yhabteab/Workspace/icinga2/prefix/etc/icinga2/zones.d/satellite' for unknown zone 'satellite'.
[2026-01-23 09:19:51 +0100] warning/config: Ignoring directory '/Users/yhabteab/Workspace/icinga2/prefix/var/lib/icinga2/api/zones/master' for unknown zone 'master'.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Committing config item(s).
[2026-01-23 09:19:51 +0100] information/ApiListener: My API identity: mbp-yhabteab
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 ApiListener.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 9 CheckCommands.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 2 ApiUsers.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 Endpoint.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 CheckerComponent.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 2 Hosts.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 IcingaApplication.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 Zone.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 Service.
[2026-01-23 09:19:51 +0100] information/ConfigItem: Instantiated 1 NotificationComponent.
[2026-01-23 09:19:51 +0100] information/ScriptGlobal: Dumping variables to file '/Users/yhabteab/Workspace/icinga2/prefix/var/cache/icinga2/icinga2.vars'
[2026-01-23 09:19:51 +0100] information/cli: Finished validating the configuration file(s).
Without having looked into this in more detail, I don't understand the motivation right away. Can you please explain how this change fits into the big picture? |
@yhabteab Added a Before this PR you had to essentially either use the exact same types as Since the code for streaming does not distinguish between requests and responses, it makes sense to template the types the same way PS: personally I maybe wouldn't have merged it immediately and given you a chance to look at it too. |
I wanted to do it too and let it open for around an 1hour after my approval but nobody cared to step up ;). Seriously, I wanted to answer Julian's question after I rebased my PR and make use of this, so that he can get the prove what this is useful for but thanks anyway. It's nothing that can't be undone, so 🤷! |
|
So the otel writer is supposed to use chunked encoding (which we recently implemented for our REST API responses), so that was generalized to support it for both sending requests and responses. And otel probably allows plaintext HTTP as well, so the underlying stream type was made a template as well? Anything more that I've missed? |
Yes. The Protobuf messages are streamed in chunks, so I had to add a kinda exact replica of the existing
Not just otel but all the other writers too but our API requests/responses on the other hand use always TLS stream. But yes, that's exactly the reason.
Nope, that pretty much summs it up. |
This adds generalized IncomingHttpMessage and OutgoingHttpMessage templates that support different types of streams (via a std::variant) and can both be used for either requests or responses.
The tacked on metadata from the old HttpRequest and server connection from the old HttpServerConnection have been moved to HttpApi(Request|Response) classes that derive from the above generalized message types.
The implementation has been left in the source file so templating the classes doesn't blow up the compile times too much. This has the caveat that any new code that wants to use different instances of the template will have to add their instantiation to
httpmessage.cpp, but I think it's better this way.I wanted to do something like this initially in #10516, but as at that time nothing else would have used these classes and the PR was already huge I tried to reduce complexity.
This is useful for #10685, and maybe #10668. Since this doesn't have any functional changes it's better to have it here and review/merge it separately.
PS: I've split up the commits into one that only renames
Http(Request|Response)toHttpApi(Request|Response)and one that adds the actual generalization of the underlying class, for easier review. I also suggest to turn on "Hide Whitespace" as it makes the diff much easier to read.