From 612139aa92ddbbcef8a90c1b179754856a006cf0 Mon Sep 17 00:00:00 2001 From: Alexey Zatelepin Date: Tue, 23 Jul 2024 14:43:21 +0200 Subject: [PATCH 1/3] http: add clarifying comment about the client::stop method (cherry picked from commit 31f127ccd297a9c46c307a8b56358d44b16824c7) --- src/v/http/include/http/client.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/v/http/include/http/client.h b/src/v/http/include/http/client.h index ca993bb966a03..09955855b18e6 100644 --- a/src/v/http/include/http/client.h +++ b/src/v/http/include/http/client.h @@ -84,6 +84,7 @@ class client : protected net::base_transport { ss::shared_ptr probe, ss::lowres_clock::duration max_idle_time = {}); + /// Stop must be called before destroying the client object. ss::future<> stop(); using net::base_transport::shutdown; using net::base_transport::wait_input_shutdown; From 8b20c1dc79b17629a26125b21fcff66032811f57 Mon Sep 17 00:00:00 2001 From: Alexey Zatelepin Date: Tue, 23 Jul 2024 14:46:29 +0200 Subject: [PATCH 2/3] c/metrics_reporter: ensure http client is stopped before destroying In case of exceptions, destroying the client may not be safe because the underlying output stream may not be fully flushed. (cherry picked from commit 35fa28ba002a4511b33991099fc5d488a65a1a1e) --- src/v/cluster/metrics_reporter.cc | 35 +++++++++++++++++-------------- src/v/cluster/metrics_reporter.h | 1 + 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/v/cluster/metrics_reporter.cc b/src/v/cluster/metrics_reporter.cc index 67ab5e3c1f19b..4d4292a17860d 100644 --- a/src/v/cluster/metrics_reporter.cc +++ b/src/v/cluster/metrics_reporter.cc @@ -396,6 +396,21 @@ ss::future metrics_reporter::make_http_client() { co_return http::client(client_configuration, _as.local()); } +ss::future<> +metrics_reporter::do_send_metrics(http::client& client, iobuf body) { + auto timeout = config::shard_local_cfg().metrics_reporter_tick_interval(); + auto res = co_await client.get_connected(timeout, _logger); + // skip sending metrics, unable to connect + if (res != http::reconnect_result_t::connected) { + vlog( + _logger.trace, "unable to send metrics report, connection timeout"); + co_return; + } + auto resp_stream = co_await client.post( + _address.path, std::move(body), http::content_type::json, timeout); + co_await resp_stream->prefetch_headers(); +} + ss::future<> metrics_reporter::do_report_metrics() { // try initializing cluster info, if it is already present this operation // does nothing. @@ -451,22 +466,10 @@ ss::future<> metrics_reporter::do_report_metrics() { } auto out = serialize_metrics_snapshot(snapshot.value()); try { - // prepare http client - auto client = co_await make_http_client(); - auto timeout - = config::shard_local_cfg().metrics_reporter_tick_interval(); - auto res = co_await client.get_connected(timeout, _logger); - // skip sending metrics, unable to connect - if (res != http::reconnect_result_t::connected) { - vlog( - _logger.trace, - "unable to send metrics report, connection timeout"); - co_return; - } - auto resp_stream = co_await client.post( - _address.path, std::move(out), http::content_type::json, timeout); - co_await resp_stream->prefetch_headers(); - co_await resp_stream->shutdown(); + co_await http::with_client( + co_await make_http_client(), [this, &out](http::client& client) { + return do_send_metrics(client, std::move(out)); + }); _last_success = ss::lowres_clock::now(); } catch (...) { vlog( diff --git a/src/v/cluster/metrics_reporter.h b/src/v/cluster/metrics_reporter.h index 326c7322db54a..9343276927df2 100644 --- a/src/v/cluster/metrics_reporter.h +++ b/src/v/cluster/metrics_reporter.h @@ -103,6 +103,7 @@ class metrics_reporter { ss::future> build_metrics_snapshot(); ss::future make_http_client(); + ss::future<> do_send_metrics(http::client&, iobuf body); ss::future<> try_initialize_cluster_info(); ss::future<> propagate_cluster_id(); From d55f5a356eeafc8031353db9df720d50f07fbeff Mon Sep 17 00:00:00 2001 From: Alexey Zatelepin Date: Tue, 23 Jul 2024 22:22:36 +0200 Subject: [PATCH 3/3] http: delete unused client::response_stream::shutdown() method It just calls client->stop() (which will make the client unusable and we need to call anyway before the client is destroyed), so remove it to avoid confusion. (cherry picked from commit 59a42de66f09b51593197d043087e687ce4bf37b) --- src/v/http/client.cc | 2 -- src/v/http/include/http/client.h | 3 --- 2 files changed, 5 deletions(-) diff --git a/src/v/http/client.cc b/src/v/http/client.cc index fad4556e03490..a7d805c62083d 100644 --- a/src/v/http/client.cc +++ b/src/v/http/client.cc @@ -287,8 +287,6 @@ iobuf_to_constbufseq(const iobuf& iobuf) { return seq; } -ss::future<> client::response_stream::shutdown() { return _client->stop(); } - /// Return failed future if ec is set, otherwise return future in ready state static ss::future fail_on_error(prefix_logger& ctxlog, const boost::beast::error_code& ec) { diff --git a/src/v/http/include/http/client.h b/src/v/http/include/http/client.h index 09955855b18e6..735fb09cffcd7 100644 --- a/src/v/http/include/http/client.h +++ b/src/v/http/include/http/client.h @@ -109,9 +109,6 @@ class client : protected net::base_transport { response_stream& operator=(response_stream const&) = delete; response_stream operator=(response_stream&&) = delete; - /// \brief Shutdown connection gracefully - ss::future<> shutdown(); - /// Return true if the whole http payload is received and parsed bool is_done() const;