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
11 changes: 11 additions & 0 deletions include/seastar/http/exception.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,17 @@ public:
{}
};

/**
* Client-side exception to report that making request timed out
* prior to communicating to server
*/
class timeout_error : public base_exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a class with exactly the same name (but different definition) in rpc_types.hh, and the very similar name timed_out_error in include/seastar/core/timed_out_error.hh. All this is very confusing for users of the library.

I think the name of the HTTP exception type should closely follow the name of the HTTP status it sets. I would call it request_timeout_exception. Similar to the not_found_exception and bad_request_exception we already have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I looked at the last exception in this header which is unex pected_status_error and named the new class with _error suffix. Will rename.

public:
timeout_error()
: base_exception("client timed out", http::reply::status_type::request_timeout)
{}
};

SEASTAR_MODULE_EXPORT_END
}

Expand Down
20 changes: 18 additions & 2 deletions src/http/client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module;
#include <optional>
#include <stdexcept>
#include <utility>
#include <chrono>

#ifdef SEASTAR_MODULE
module seastar;
Expand Down Expand Up @@ -252,7 +253,14 @@ future<client::connection_ptr> client::get_connection(clock_type::time_point tim
}

if (_nr_connections >= _max_connections) {
return _wait_con.wait().then([this, timeout] {
return _wait_con.wait(timeout).then_wrapped([this, timeout] (auto f) {
try {
f.get();
} catch (const condition_variable_timed_out&) {
return make_exception_future<connection_ptr>(httpd::timeout_error());
} catch (...) {
return current_exception_as_future<connection_ptr>();
}
return get_connection(timeout);
});
}
Expand All @@ -262,9 +270,17 @@ future<client::connection_ptr> client::get_connection(clock_type::time_point tim

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 {
return _new_connections->make().then([this, timeout, cr = internal::client_ref(this)] (connected_socket cs) mutable {
http_log.trace("created new http connection {}", cs.local_address());
auto con = seastar::make_shared<connection>(std::move(cs), std::move(cr));
if (clock_type::now() > timeout) {
// Factory made new connection, though it's too late already. Don't throw
// this effort out, but don't use it to serve current request either.
return put_connection(std::move(con)).then_wrapped([] (auto f) {
f.ignore_ready_future();
return make_exception_future<connection_ptr>(httpd::timeout_error());
});
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 understand why we need this super-rare special case. If you set a 1 minute connection timeout, and the connection was made in 59.999 seconds but then when you want to start sending it's already 60.001 seconds - why do we need to treat it specially? Remember that if we start sending in 59.999, then the timeout is no longer relevant - we could be sending 1 TB of data and the whole thing will succeed.

So I would simply remove this piece of code. If the make_connection() said it finished by the timeout, that's fine - we don't need to second-guess its decision here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Factory doesn't know this deadline and it can generate new connection in much more than 59.999 seconds easily and there are two reasons for that.

First -- the call to make new connection can happen after client tried serving request over polled one (#1883). E.g. -- timeout is 1 minute, client waits 30 seconds for pooled connection, then starts sending data over it and in 15 seconds server closes its end. Client asks for a new connection and it takes 1 minute to establish it. Boom -- 105 seconds had passed.

Second -- we may equip factory::make() with timeout parameter too, but connect() is not always abortable nowadays (#2302). It's a BUG, but still http client has means to handle it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The example you gave looks wrong to me. Remember that the timeout is an absolute time, not "1 minute". So if the client already waited for 30 seconds for a pool connection and then asks for a new connection, this new connection will have the same deadline and will only have 30 more seconds to complete the connection, not one minute.

I don't know what you mean that connect() is not abortable - you mean it doesn't obey timeout? Then what's the point of this whole patch? (I also asked this regarding the test!).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'm starting to think that this specific if() that I didn't like is actually the main part of your patch. Apparently, you really care if the connection attempt takes more than the given amount of time (and you even said in reponses to my questions, but not in the cover letter, that you can't control the timeout in a real system connect()). All you care is that if the connection attempt takes more than the given timeout - the request will not be sent because it is already considered stale.

So this patch is apparently about not sending stale requests, more about the ability to time out. I now understand that you consider it fine to wait 10 minutes instead of the 1 minute timeout, as long as after that 10 minutes we don't send the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The thing is that client doesn't establishes new connections itself, it calls connection_factory::make() and doesn't have any control over it. Of course, we can equip connection_factory::make() with timeout argument and assume that it doesn't ignore it (the whole client is still experimental, so we're relatively safe in changing the API like that), but should client fulfill its API contract and not send a request even if factory didn't fulfill its and missed the timeout?

}
return make_ready_future<connection_ptr>(std::move(con));
});
}
Expand Down