From 31f127ccd297a9c46c307a8b56358d44b16824c7 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 --- 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 35fa28ba002a4511b33991099fc5d488a65a1a1e 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. --- 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 13467346639d2..0a73e0a472054 100644 --- a/src/v/cluster/metrics_reporter.cc +++ b/src/v/cluster/metrics_reporter.cc @@ -398,6 +398,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. @@ -453,22 +468,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 59a42de66f09b51593197d043087e687ce4bf37b 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. --- 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;