Skip to content
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

Jump to bottom

Teach http::client::make_request() to timeout #2304

Closed
wants to merge 4 commits into from
Closed

Teach http::client::make_request() to timeout #2304

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b3270e
http/client: Add timeout argument to make_request()
xemul Jun 26, 2024
e0d0324
http/client: Respect user-provided timeout
xemul Jun 26, 2024
5281ee2
test/http: Generalize simple request sending
xemul Jun 26, 2024
8eedc90
test/http: Test how client::make_request() handles provided timeout
xemul Jun 26, 2024
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
13 changes: 8 additions & 5 deletions include/seastar/http/client.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <seastar/http/reply.hh>
#include <seastar/core/condition-variable.hh>
#include <seastar/core/iostream.hh>
#include <seastar/core/lowres_clock.hh>
#include <seastar/util/modules.hh>

namespace bi = boost::intrusive;
Expand Down Expand Up @@ -169,6 +170,7 @@ class client {
public:
using reply_handler = noncopyable_function<future<>(const reply&, input_stream<char>&& body)>;
using retry_requests = bool_class<struct retry_requests_tag>;
using clock_type = lowres_clock;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this name "clock_type". It's very unclear what this clock is used for. Future developers may start using it for the wrong things (e.g., if you want to measure a latency histogram, you may not want to use lowres_clock to do this).
I think you should name it "timeout_clock" instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I followed the rpc_clock_type "naming convention" for no real reason


private:
friend class http::internal::client_ref;
Expand All @@ -185,17 +187,17 @@ private:

using connection_ptr = seastar::shared_ptr<connection>;

future<connection_ptr> get_connection();
future<connection_ptr> make_connection();
future<connection_ptr> get_connection(clock_type::time_point timeout);
future<connection_ptr> make_connection(clock_type::time_point timeout);
future<> put_connection(connection_ptr con);
future<> shrink_connections();

template <std::invocable<connection&> Fn>
auto with_connection(Fn&& fn);
auto with_connection(Fn&& fn, clock_type::time_point timeout);

template <typename Fn>
requires std::invocable<Fn, connection&>
auto with_new_connection(Fn&& fn);
auto with_new_connection(Fn&& fn, clock_type::time_point timeout);

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

Expand Down Expand Up @@ -275,12 +277,13 @@ public:
* \param req -- request to be sent
* \param handle -- the response handler
* \param expected -- the optional expected reply status code, default is std::nullopt
* \param send_timeout -- time point at which make_request will stop waiting for transport
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be clearer and less fuzzy what this timeout limits. I would call it a "connect timeout" (not "send_timeout", I think this name is wrong). I would explicitly say it is a timeout for making the initial connection but establishing a connection, it does not limit the time the request takes - and name this variable "connect_timeout".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine to rename it, but I'd like to emphasize that connect timeout doesn't fit well either. As you noticed below, this timeout also applies to the cond. variable wait() which waits for existing connection (from pool) to become available, and it doesn't imply connecting by any means. From the code perspective, this timeout separates the time client spends obtaining a connection (new one or from poll) and the process of sending request head. So this is sort of "request head sending timeout", but I've no better idea for its name that that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a timeout waiting for a connection - not necessarily a new connection!

This distinction can be explained in text. But the name "send_timeout" suggests it's a timeout until the entire request is sent. This is not what you are doing. Moreover, if the request is very large (e.g., uploading a file), it is very far from what you're doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a timeout waiting for a connection - not necessarily a new connection!

🤣 that's 100% perfect explanation, all the more so the object that client waits for is indeed class connection. I'll rename

*
* Note that the handle callback should be prepared to be called more than once, because
* client may restart the whole request processing in case server closes the connection
* in the middle of operation
*/
future<> make_request(request req, reply_handler handle, std::optional<reply::status_type> expected = std::nullopt);
future<> make_request(request req, reply_handler handle, std::optional<reply::status_type> expected = std::nullopt, clock_type::time_point send_timeout = clock_type::time_point::max());

/**
* \brief Updates the maximum number of connections a client may have
Expand Down
28 changes: 14 additions & 14 deletions src/http/client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ client::client(std::unique_ptr<connection_factory> f, unsigned max_connections,
{
}

future<client::connection_ptr> client::get_connection() {
future<client::connection_ptr> client::get_connection(clock_type::time_point timeout) {
if (!_pool.empty()) {
connection_ptr con = _pool.front().shared_from_this();
_pool.pop_front();
Expand All @@ -252,15 +252,15 @@ future<client::connection_ptr> client::get_connection() {
}

if (_nr_connections >= _max_connections) {
return _wait_con.wait().then([this] {
return get_connection();
return _wait_con.wait().then([this, timeout] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't _wait_con.wait() also need to obey "timeout"?

In fact, you explicitly gave this example in your commit message: Some highly parallel application where requests sit in a queue for 15 minutes before they even start, i.e. - before get_connect() finishes waiting for the connection. So we need to add the 15 minute limit also on this wait.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see you fix this in the next patch. Good.
For me, this sort of patch splitting (with no hint that you did that in the commit message) just makes the reviewer harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree with "with no hint" credit :D here's the phrase from this commit description:

Next patch will make the code obey the provided timeout.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't understand this. I thought I saw some of the things already obeying the timeout.

return get_connection(timeout);
});
}

return make_connection();
return make_connection(timeout);
}

future<client::connection_ptr> client::make_connection() {
future<client::connection_ptr> client::make_connection(clock_type::time_point timeout) {
_total_new_connections++;
return _new_connections->make().then([cr = internal::client_ref(this)] (connected_socket cs) mutable {
http_log.trace("created new http connection {}", cs.local_address());
Expand Down Expand Up @@ -311,8 +311,8 @@ future<> client::set_maximum_connections(unsigned nr) {
}

template <std::invocable<connection&> Fn>
auto client::with_connection(Fn&& fn) {
return get_connection().then([this, fn = std::move(fn)] (connection_ptr con) mutable {
auto client::with_connection(Fn&& fn, clock_type::time_point timeout) {
return get_connection(timeout).then([this, fn = std::move(fn)] (connection_ptr con) mutable {
return fn(*con).finally([this, con = std::move(con)] () mutable {
return put_connection(std::move(con));
});
Expand All @@ -321,22 +321,22 @@ auto client::with_connection(Fn&& fn) {

template <typename Fn>
requires std::invocable<Fn, connection&>
auto client::with_new_connection(Fn&& fn) {
return make_connection().then([this, fn = std::move(fn)] (connection_ptr con) mutable {
auto client::with_new_connection(Fn&& fn, clock_type::time_point timeout) {
return make_connection(timeout).then([this, fn = std::move(fn)] (connection_ptr con) mutable {
return fn(*con).finally([this, con = std::move(con)] () mutable {
return put_connection(std::move(con));
});
});
}

future<> client::make_request(request req, reply_handler handle, std::optional<reply::status_type> expected) {
return do_with(std::move(req), std::move(handle), [this, expected] (request& req, reply_handler& handle) mutable {
future<> client::make_request(request req, reply_handler handle, std::optional<reply::status_type> expected, clock_type::time_point send_timeout) {
return do_with(std::move(req), std::move(handle), [this, expected, send_timeout] (request& req, reply_handler& handle) mutable {
auto f = with_connection([this, &req, &handle, expected] (connection& con) {
return do_make_request(con, req, handle, expected);
});
}, send_timeout);

if (_retry) {
f = f.handle_exception_type([this, &req, &handle, expected] (const std::system_error& ex) {
f = f.handle_exception_type([this, &req, &handle, expected, send_timeout] (const std::system_error& ex) {
auto code = ex.code().value();
if ((code != EPIPE) && (code != ECONNABORTED)) {
return make_exception_future<>(ex);
Expand All @@ -347,7 +347,7 @@ future<> client::make_request(request req, reply_handler handle, std::optional<r
// break the limit. That's OK, the 'con' will be closed really soon
return with_new_connection([this, &req, &handle, expected] (connection& con) {
return do_make_request(con, req, handle, expected);
});
}, send_timeout);
});
}

Expand Down