diff --git a/CHANGELOG.md b/CHANGELOG.md index 805c832e5..2917b1a63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ Increment the: * [CONFIGURATION] Add a configuration builder for the host resource detector [#4451](https://github.com/open-telemetry/opentelemetry-cpp/issues/4451) +* [BUGFIX] Stop a curl request whose gzip step failed, instead of sending + a body the failed compression had already overwritten + [#4360](https://github.com/open-telemetry/opentelemetry-cpp/issues/4360) * [CONFIGURATION] Build the configured resource detectors in SdkBuilder, apply the `detection.attributes` include/exclude filter to the detected attributes, and merge the resource per the resource SDK specification. diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 87f2c123a..d900c74b1 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -184,12 +184,22 @@ void Session::SendRequest( if (stream != Z_OK) { + // zs.msg points into the stream deflateEnd() releases. + const std::string reason = (nullptr != zs.msg) ? zs.msg : ""; + deflateEnd(&zs); + + // The handler may start another request on this session or drop the last reference to it, + // so nothing below this point may touch the session. + is_session_active_.store(false, std::memory_order_release); + if (callback) { - callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, - zs.msg ? zs.msg : ""); + callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, reason); } - is_session_active_.store(false, std::memory_order_release); + + // deflateInPlace() rewrote part of the body before reporting that it would not fit, and no + // Content-Encoding header describes what is left. The request does not go out. + return; } deflateEnd(&zs); diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 90142962d..5646a9b4c 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -984,6 +984,46 @@ struct GzipEventHandler : public CustomEventHandler std::string reason_; }; +// A request whose compression step fails reports CreateFailed and is not sent. +// +// The failure is arranged, not injected: deflateInPlace() gets the body's own size as its output +// budget, and one byte cannot hold a gzip header, so it returns Z_BUF_ERROR. +TEST_F(BasicCurlHttpTests, AFailedCompressionStopsTheRequest) +{ + received_requests_.clear(); + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("post/"); + request->SetMethod(http_client::Method::Post); + + http_client::Body body(1); + request->SetBody(body); + request->AddHeader("Content-Type", "text/plain"); + request->SetCompression(opentelemetry::ext::http::client::Compression::kGzip); + + auto handler = std::make_shared(); + session->SendRequest(handler); + + // Asserted rather than assumed: a change that made this body compressible would otherwise leave + // the case passing while testing nothing. + ASSERT_TRUE(handler->is_called_) << "the compression did not fail, so nothing was tested"; + ASSERT_EQ(handler->state_, http_client::SessionState::CreateFailed); + + session->FinishSession(); + session_manager->FinishAllSessions(); + + // Long enough that a request which was going to be sent has been. The server records every + // request it receives, including one whose body is unreadable. + std::this_thread::sleep_for(std::chrono::milliseconds{500}); + + std::unique_lock lk1(mtx_requests); + EXPECT_TRUE(received_requests_.empty()) + << "a request the caller was told had failed was sent anyway"; +} + TEST_F(BasicCurlHttpTests, GzipCompressibleData) { received_requests_.clear(); @@ -1067,26 +1107,26 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) 63, 35, 21, 121, 152, 22, 242, 199, 106, 217, 199, 211, 206, 165, 88, 77, 112, 108, 193, 122, 8, 193, 74, 91, 50, 6, 156, 185, 165, 15, 92, 116, 3, 18, 244, 165, 191, 2, 183, 9, 164, 116, 75, 127}; - const auto original_size = body.size(); request->SetBody(body); request->AddHeader("Content-Type", "text/plain"); request->SetCompression(opentelemetry::ext::http::client::Compression::kGzip); auto handler = std::make_shared(); session->SendRequest(handler); - ASSERT_TRUE(waitForRequests(30, 1)); - session->FinishSession(); - ASSERT_TRUE(handler->is_called_); - ASSERT_EQ(handler->state_, http_client::SessionState::Response); - ASSERT_TRUE(handler->reason_.empty()); - auto http_request = - dynamic_cast(request.get()); - ASSERT_TRUE(http_request != nullptr); - ASSERT_EQ(http_request->body_.size(), original_size); + // deflateInPlace() overwrites part of the caller's buffer before reporting that the result will + // not fit, so no payload survives to send uncompressed. Whether to keep one is #4360. + ASSERT_TRUE(handler->is_called_); + ASSERT_EQ(handler->state_, http_client::SessionState::CreateFailed); + session->FinishSession(); session_manager->CancelAllSessions(); session_manager->FinishAllSessions(); + + std::this_thread::sleep_for(std::chrono::milliseconds{500}); + std::unique_lock lk1(mtx_requests); + EXPECT_TRUE(received_requests_.empty()) + << "a body the compression step had already overwritten was sent anyway"; } #endif // ENABLE_OTLP_COMPRESSION_PREVIEW