Skip to content

Fix and tune http::request setup by client #2934

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 1, 2025
Merged

Fix and tune http::request setup by client #2934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
072e86f
http: Setup request once
xemul Aug 15, 2025
1d96dac
http: Move request version configuration
xemul Aug 15, 2025
b68ed89
http: Move request content length header setup
xemul Aug 15, 2025
c7709fb
http: Make request making API return exceptional future not throw
xemul Aug 15, 2025
4267526
http: Make client::make_request accept const request&
xemul Aug 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions include/seastar/http/client.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ public:
future<> close();

private:
future<reply_ptr> do_make_request(request& rq);
void setup_request(request& rq);
future<reply_ptr> do_make_request(const request& rq);
future<> send_request_head(const request& rq);
future<reply_ptr> maybe_wait_for_continue(const request& req);
future<> write_body(const request& rq);
Expand Down Expand Up @@ -182,7 +181,7 @@ private:
requires std::invocable<Fn, connection&>
auto with_new_connection(Fn&& fn, abort_source*);

future<> do_make_request(connection& con, request& req, reply_handler& handle, abort_source*, std::optional<reply::status_type> expected);
future<> do_make_request(connection& con, const request& req, reply_handler& handle, abort_source*, std::optional<reply::status_type> expected);

public:
/**
Expand Down Expand Up @@ -275,7 +274,7 @@ public:
* `request and the `handle`, it caller's responsibility the make sure they
* are referencing valid instances
*/
future<> make_request(request& req, reply_handler& handle, std::optional<reply::status_type> expected = std::nullopt, abort_source* as = nullptr);
future<> make_request(const request& req, reply_handler& handle, std::optional<reply::status_type> expected = std::nullopt, abort_source* as = nullptr);

/**
* \brief Updates the maximum number of connections a client may have
Expand Down
28 changes: 17 additions & 11 deletions src/http/client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,12 @@ future<connection::reply_ptr> connection::maybe_wait_for_continue(const request&
});
}

void connection::setup_request(request& req) {
static void validate_request(const request& req) {
if (req._version.empty()) {
req._version = "1.1";
throw std::runtime_error("HTTP version not set");
Copy link
Contributor

@nyh nyh Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@xemul xemul Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nyh nyh Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@xemul xemul Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

@nyh nyh Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (req.content_length != 0) {
if (!req.body_writer && req.content.empty()) {
throw std::runtime_error("Request body writer not set and content is empty");
}
req._headers["Content-Length"] = to_sstring(req.content_length);
if (req.content_length != 0 && !req.body_writer && req.content.empty()) {
throw std::runtime_error("Request body writer not set and content is empty");
}
}

Expand Down Expand Up @@ -156,8 +153,7 @@ future<connection::reply_ptr> connection::recv_reply() {
});
}

future<connection::reply_ptr> connection::do_make_request(request& req) {
setup_request(req);
future<connection::reply_ptr> connection::do_make_request(const request& req) {
return send_request_head(req).then([this, &req] {
return maybe_wait_for_continue(req).then([this, &req] (reply_ptr cont) {
if (cont) {
Expand All @@ -174,6 +170,11 @@ future<connection::reply_ptr> connection::do_make_request(request& req) {
}

future<reply> connection::make_request(request req) {
try {
validate_request(req);
} catch (...) {
return current_exception_as_future<reply>();
Copy link
Contributor

@nyh nyh Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-(

Copy link
Contributor Author

@xemul xemul Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2b81a9:

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.

Copy link
Contributor

@nyh nyh Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.

Copy link
Contributor

@nyh nyh Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

}
return do_with(std::move(req), [this] (auto& req) {
return do_make_request(req).then([] (reply_ptr rep) {
return make_ready_future<reply>(std::move(*rep));
Expand Down Expand Up @@ -353,7 +354,12 @@ static bool is_retryable_exception(std::exception_ptr ex) {
return false;
}

future<> client::make_request(request& req, reply_handler& handle, std::optional<reply::status_type> expected, abort_source* as) {
future<> client::make_request(const request& req, reply_handler& handle, std::optional<reply::status_type> expected, abort_source* as) {
try {
validate_request(req);
} catch (...) {
return current_exception_as_future();
}
return with_connection([this, &req, &handle, as, expected] (connection& con) {
return do_make_request(con, req, handle, as, expected);
}, as).handle_exception([this, &req, &handle, as, expected] (std::exception_ptr ex) {
Expand Down Expand Up @@ -391,7 +397,7 @@ class skip_body_source : public data_source_impl {
}
};

future<> client::do_make_request(connection& con, request& req, reply_handler& handle, abort_source* as, std::optional<reply::status_type> expected) {
future<> client::do_make_request(connection& con, const request& req, reply_handler& handle, abort_source* as, std::optional<reply::status_type> expected) {
auto sub = as ? as->subscribe([&con] () noexcept { con.shutdown(); }) : std::nullopt;
return con.do_make_request(req).then([this, &con, &req, &handle, expected] (connection::reply_ptr reply) mutable {
auto& rep = *reply;
Expand Down
3 changes: 3 additions & 0 deletions src/http/request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ sstring request::parse_query_param() {
void request::write_body(const sstring& content_type, sstring content) {
set_content_type(content_type);
content_length = content.size();
_headers["Content-Length"] = to_sstring(content_length);
this->content = std::move(content);
}

Expand All @@ -113,6 +114,7 @@ void request::write_body(const sstring& content_type, body_writer_type&& body_wr
void request::write_body(const sstring& content_type, size_t len, body_writer_type&& body_writer) {
set_content_type(content_type);
content_length = len;
_headers["Content-Length"] = to_sstring(content_length);
if (len > 0) {
// At the time of this writing, connection::write_body()
// assumes that `body_writer` is unset if `content_length` is 0.
Expand All @@ -126,6 +128,7 @@ void request::set_expects_continue() {

request request::make(sstring method, sstring host, sstring path) {
request rq;
rq._version = "1.1";
rq._method = std::move(method);
rq._url = std::move(path);
rq._headers["Host"] = std::move(host);
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/httpd_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2099,7 +2099,7 @@ SEASTAR_THREAD_TEST_CASE(test_http_with_broken_wire) {

http::experimental::client c(addr, creds);
http::request req;

req._version = "1.1";
req.write_body("html", std::string(5134, 'a'));

auto sa = server.accept();
Expand Down