Make http::client::make_request() abortable #2410
Conversation
xemul
force-pushed
the
br-http-client-abortable-make-request
branch
from
89f28fb
to
25e5e18
Compare
|
upd:
|
| @@ -349,9 +350,6 @@ future<> client::make_request(request req, reply_handler handle, std::optional<r | |||
| return do_make_request(con, req, handle, expected); | |||
| }); | |||
| }); | |||
| } | |||
|
|
|||
| return f; | |||
There was a problem hiding this comment.
A coroutine would be much better here
| }); | ||
| }); | ||
| } | ||
|
|
||
| future<> client::do_make_request(connection& con, request& req, reply_handler& handle, std::optional<reply::status_type> expected) { | ||
| future<> client::do_make_request(connection& con, request& req, reply_handler& handle, abort_source* as, std::optional<reply::status_type> expected) { | ||
| return con.do_make_request(req).then([&con, &handle, expected] (connection::reply_ptr reply) mutable { |
There was a problem hiding this comment.
Does it not make sense to share the abort_source in the client class?
Why does each request need a separate abort_source?
There was a problem hiding this comment.
Because the intent is to abort individual make_request, not the whole client. Scylla uses single client for configured S3 endpoint. If there's a keyspace with storage=s3 and backup activity towards the same endpoint, if user wants to abort only backup, it should abort only some subset of in-flight requests, not the whole client.
There was a problem hiding this comment.
Why not create a client per {set of related requests}?
There was a problem hiding this comment.
Then we'll need to implement resource control in a different manner. Resources in the scope of s3 client are -- memory buffers (we flush sstables in 5Mb parts) and the number of sockets in client pool. Or leave different {sets of related requests} consume resources independently
There was a problem hiding this comment.
@avikivity , reminder ping
There was a problem hiding this comment.
Sorry. Could we extract the resource controller out? So multiple clients can share the same resource pool?
Though it's not out of the question to want to abort a particular request.
There was a problem hiding this comment.
Sorry. Could we extract the resource controller out? So multiple clients can share the same resource pool?
Yes, it's possible. However, http::client is all about socket pool management, so sharing sockets pool is the same as sharing the http client itself.
Though it's not out of the question to want to abort a particular request.
Yes. Also, putting abort source on client will not change much in the code, just instead of abort_source& argument there will be this->abort_source
After calling connection::do_make_request() client may attch a continuation to it that handles EPIPE/ECONNABORTED and retries the request. That "may" depends o whether retries were requested in client. This patch makes the continuation unconditional, next patched will need it even for non-retriable requests. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This just propagates abort_source* argument to some calls in client. The argument is unused and is, in fact, nullptr all the time. Next patches will change that. This change is just to reduce further churn. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
One of the places where client can sleep and may need to be aborted is making new connection via factory method. Add the abort source argument there. This breaks the API, but it's still experimental, so fine. Also, existing sample factories ignore the abort source. This will be done as followup. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Another place where client may sleep is when waiting for free connection from pool. Wait happens on conditional variable, but it cannot be aborted (see scylladb#2013). So instead, subscribe for the abort source and wake up all the waiters. Then check the local abort source for being aborted in the continuation. Those waiters that are not aborted will re-enter and will sleep again. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
When a socket is doing request-response cycle, aborting it can only be done by shutting down and eliminating from the pool. For that -- subscribe on the abort source with the callback that marks the connection as no longer persistent and shuts it down. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Everything is ready, now add the make_request() that accepts abort source reference and passes it along the stack. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Test all four scenarios: - abort establishing a connection - abort waiting for connection from pool - abort conneciton that sends request - abort connection that waits for response Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
xemul
force-pushed
the
br-http-client-abortable-make-request
branch
from
25e5e18
to
928bd93
Compare
|
upd:
|
| @@ -150,7 +152,7 @@ public: | |||
| * The implementations of this method should return ready-to-use socket that will | |||
| * be used by \ref client as transport for its http connections | |||
| */ | |||
| virtual future<connected_socket> make() = 0; | |||
| virtual future<connected_socket> make(abort_source*) = 0; | |||
There was a problem hiding this comment.
this broke the build of scylla
In file included from /home/kefu/dev/scylladb/tools/scylla-nodetool.cc:52:
/home/kefu/dev/scylladb/utils/http.hh:67:38: error: 'make' marked 'override' but does not override any member functions
67 | virtual future<connected_socket> make() override {
| ^
In file included from /home/kefu/dev/scylladb/tools/scylla-nodetool.cc:11:
In file included from /usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/chrono:49:
In file included from /usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/bits/shared_ptr.h:53:
In file included from /usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/bits/shared_ptr_base.h:59:
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/bits/unique_ptr.h:1076:34: error: allocating an object of abstract class type 'utils::http::dns_connection_factory'
1076 | { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
| ^
/home/kefu/dev/scylladb/tools/scylla-nodetool.cc:185:28: note: in instantiation of function template specialization 'std::make_unique<utils::http::dns_connection_factory, seastar::basic_sstring<char, unsigned int, 15> &, unsigned short &, bool, seastar::logger &>' requested here
185 | , _api_client(std::make_unique<utils::http::dns_connection_factory>(_host, _port, false, nlog), 1)
| ^
/home/kefu/dev/scylladb/seastar/include/seastar/http/client.hh:155:38: note: unimplemented pure virtual method 'make' in 'dns_connection_factory'
155 | virtual future<connected_socket> make(abort_source*) = 0;
| ^
2 errors generated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This PR adds abort_source& argument to client::make_request() method. With it, calling abort_source::request_abort() will break the request making process wherever it is and resolve the make_request() with abort_requested_exception exception.
One of the use-cases is to let client impose a timeout on the make_request(). For that, a timer should be armed that would request abort on the abort source passed to the method. In #2304 there was an attempt to put a timeout on connect() only, but this approach is more generic and allows aborting request at any stage, not only connecting.
Another scenario is stopping long operations on user request. There can be several examples of what "long" operation can be. One of them is simple connect() to the server -- TCP times out after 1 minute by default, and breaking this earlier can be useful. Another example of long operation is S3 file uploading. It may happen in "parts" and part can be of any size, so simple writing of the request body into the socket may take time.
fixes: #2409
closes: #2304