Fix and tune http::request setup by client #2934
Conversation
The setup_request() is now called from connection::do_make_request() which, in turn, can be called by client several time, once per "retry". Move the setup_request() invocation up the stack so that request is set up once per make_request() call. This allows marking some request making methods as const request& Signed-off-by: Pavel Emelyanov <[email protected]>
When making a request without version, client sets 1.1 by default. All (but one test) the users of client create request with make() helper, so the version configuration can be moved there. This will allow to make request making API accept const references on request in the future. Signed-off-by: Pavel Emelyanov <[email protected]>
Similarly to previous patch, the content is placed on request with the help of write_body() helpers, so thay can setup the necessary headers on their own. This makes it symmatrical with chunked bodies, for which content-type header is set inside write_body() helper. This will allow making request making API accept const request& soon. Signed-off-by: Pavel Emelyanov <[email protected]>
When request doesn't pass the validation, the make_request() method throws, which is not user-friendly, as the method in question returns future<>. This patch makes the method return exceptional future if validation fails. While at it -- rename setup_request() into validate_request() as this is what it does nowadays. Signed-off-by: Pavel Emelyanov <[email protected]>
It now doesn't need ot modifu the request, so the argument can be marked with const. Signed-off-by: Pavel Emelyanov <[email protected]>
|
@nyh , please review |
| static void setup_request(request& req) { | ||
| if (req._version.empty()) { | ||
| req._version = "1.1"; | ||
| throw std::runtime_error("HTTP version not set"); |
There was a problem hiding this comment.
If I understand correctly, this changes the API - you now absolutely must specify the request's version and can't have it default to 1.1 like it did. I guess it's fine to change the API because it's in the experiemental namespace?
But why do you want version to be explicitly given, since there is a very good default - 1.1 has been the default for almost 20 years now?
There was a problem hiding this comment.
If I understand correctly, this changes the API - you now absolutely must specify the request's version and can't have it default to 1.1 like it did.
It still defaults to 1.1 but in another place -- in the request::make() helper that's used by all our code anyway.
I guess it's fine to change the API because it's in the experiemental namespace?
Yes, exactly.
But why do you want version to be explicitly given, since there is a very good default - 1.1 has been the default for almost 20 years now?
Do I get it correctly that you suggest to always put "1.1" by the client regardless of what's there on the request::_version? No strong reason. However, someone is working on http/2, presumably (I didn't check that) sending a request with req._version = "2.0" and still seeing "1.1" string appearing on the socket would be quite surprising
There was a problem hiding this comment.
I may be asking a stupid question because I don't understand the API. I got the impression that the API looks something like:
request req;
request.content = "hello";
client.make_request(request)
In that case, I thought it made sense that before your patch this request will default to 1.1.
Clearly, if someone creates a request explicitly asking for a different version - that version should be used.
There was a problem hiding this comment.
The API nowadays is
auto req = request::make("GET", "google.com", "/foo/bar");
co_await client.make_request(req);
so assigning "_version = 1.1" now happens in request::make(). If someone needs another version, then it should use
auto req = request::make("GET", "google.com", "/foo/bar");
+req._version = "2.0";
co_await client.make_request(req);There was a problem hiding this comment.
Ok, though I don't understand why we have a "factory function" request::make(...) instead of an ordinary constructor, request(...). But in any case, that is not new to this patch, so I am only asking for my own education, it's not really related to this review.
| try { | ||
| validate_request(req); | ||
| } catch (...) { | ||
| return current_exception_as_future<reply>(); |
There was a problem hiding this comment.
Wow, I completely missed that we switched from the idiom
return seastar::make_exception_future(std::current_exception());
to
return current_exception_as_future<reply>();
I'm curious why. We need to update doc/tutorial.md which refers to the old idiom. Sad there is no grant any more forcing us to write such documentation :-(
There was a problem hiding this comment.
This is the future version of std::current_exception. This moves the std::current_exception call out of line, which also move the std::exception_ptr destructor call offline.
There was a problem hiding this comment.
Thanks.
To be honest, I don't understand the justification in that commit (it's the first time I'm seeing it, maybe I was on vacation in December 2019 ;-)). By "offline", did he mean "out of line"? But in any case, since both functions would be called in an exception handler*, wouldn't they be out-of-line anyway?
It's sad that that patch did not:
- Update tutorial.md. @avikivity one day we should decide to spend real resources (i.e., time) on making tutorial.md into a real book and not the 10%-written old-style-continuation thing we have today. For new employees, and perhaps we can even see Seastar really take off externally.
- Contain any sort of benchmarks or something proving why it's better to use the new idiom. Commit 0bde519 did say that half a dozen patches together reguced the code size by 1.4%, but how much of this is due to using the new current_exception_as_future()? I'll open an issue about this.
There was a problem hiding this comment.
I see the doxygen in include/seastar/core/future.hh actually says about current_exception_as_future that it is:
This is equivalent to make_exception_future(std::current_exception()), but expands to less code.
I still have no idea why and would have been nice to read about somewhere. We don't pay for doxygen comments by the byte ;-)
Yes, it's still the default, just moved to another place |
3 participants
Currently, if a request passed into client::make_request() fails validation (missing version and inconsistent body content vs headers) the method throws, while it's a furure<>-returning method and should return exceptional future instead. This PR turns exception into a future.
Other than that, request validation is moved, so that it happens once per make_request(). Nowadays it happens on every retry.
And finally, placing defaults on request is moved from connection::make_request() into request::make(), so that client request making API accepts const request reference (finalizes the #1848 effort)