Skip to content

Commit be6ff63

Browse files
juanjuxquinna-h
andauthored
fix: ensure connection is reset if from_http_response raises an exception (#15559)
## Description Previously, the http writer was calling `from_http_response` in the `else` block of the exception handler that does the request, set's the log level, et cetera. However this could have the problem of `from_http_response` itself raising an exception (e.g. `http.client.IncompleteRead` as seen on dogweb staging recently) outside of the `Exception` handler, which would leave the `HTTPWriter` object in need of a call to `_reset`; the `finally` would call `_reset` but only if `reuse_connection` aka `DD_TRACE_WRITER_REUSE_CONNECTIONS` was enabled, which is not the default. ## Testing New tests have been added. ## Risks <!-- Note any risks associated with this change, or "None" if no risks --> ## Additional Notes <!-- Any other information that would be helpful for reviewers --> --------- Signed-off-by: Juanjo Alvarez <[email protected]> Co-authored-by: Quinna Halim <[email protected]>
1 parent a15452f commit be6ff63

File tree

3 files changed

+51
-2
lines changed

3 files changed

+51
-2
lines changed

ddtrace/internal/writer/writer.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,12 +283,14 @@ def _put(self, data: bytes, headers: Dict[str, str], client: WriterClientBase, n
283283
t,
284284
self._intake_endpoint(client),
285285
)
286+
# Read response body inside try block to ensure connection
287+
# is reset if this from_http_response call throws an exception
288+
# (e.g. IncompleteRead)
289+
return Response.from_http_response(resp)
286290
except Exception:
287291
# Always reset the connection when an exception occurs
288292
self._reset_connection()
289293
raise
290-
else:
291-
return Response.from_http_response(resp)
292294
finally:
293295
# Reset the connection if reusing connections is disabled.
294296
if not self._reuse_connections:
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
Tracing, CI Visibility: Ensure the http connection is correctly reset in all error scenarios.

tests/tracer/test_writer.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -751,10 +751,32 @@ def do_POST(self):
751751
return
752752

753753

754+
class _IncompleteReadRequestHandlerTest(_BaseHTTPRequestHandler):
755+
"""Handler that sends an incomplete chunked response to simulate IncompleteRead"""
756+
757+
def do_PUT(self):
758+
# Send headers indicating chunked transfer encoding
759+
self.send_response(200)
760+
self.send_header("Transfer-Encoding", "chunked")
761+
self.end_headers()
762+
763+
# Send a partial chunk and then close the connection abruptly
764+
# This simulates the agent starting to send a response but failing midway
765+
self.wfile.write(b"5\r\n") # Chunk size indicator (5 bytes)
766+
self.wfile.write(b"Hello") # Partial chunk data
767+
# Missing: \r\n after chunk, next chunk size, and final 0\r\n\r\n
768+
# Close connection abruptly without completing the chunked response
769+
self.wfile.flush()
770+
771+
def do_POST(self):
772+
self.do_PUT()
773+
774+
754775
_HOST = "0.0.0.0"
755776
_PORT = 8743
756777
_TIMEOUT_PORT = _PORT + 1
757778
_RESET_PORT = _TIMEOUT_PORT + 1
779+
_INCOMPLETE_READ_PORT = _RESET_PORT + 1
758780

759781

760782
class UDSHTTPServer(socketserver.UnixStreamServer, http.server.HTTPServer):
@@ -827,6 +849,16 @@ def endpoint_test_reset_server():
827849
thread.join()
828850

829851

852+
@pytest.fixture(scope="module")
853+
def endpoint_test_incomplete_read_server():
854+
server, thread = _make_server(_INCOMPLETE_READ_PORT, _IncompleteReadRequestHandlerTest)
855+
try:
856+
yield thread
857+
finally:
858+
server.shutdown()
859+
thread.join()
860+
861+
830862
@pytest.fixture
831863
def endpoint_assert_path():
832864
handler = _APIEndpointRequestHandlerTest
@@ -899,6 +931,17 @@ def test_flush_connection_reset(endpoint_test_reset_server, writer_class):
899931
writer.flush_queue(raise_exc=True)
900932

901933

934+
@pytest.mark.parametrize("writer_class", (AgentWriter, CIVisibilityWriter, NativeWriter))
935+
def test_flush_connection_incomplete_read(endpoint_test_incomplete_read_server, writer_class):
936+
"""Test that IncompleteRead errors are handled properly by resetting the connection"""
937+
writer = writer_class(f"http://{_HOST}:{_INCOMPLETE_READ_PORT}")
938+
# IncompleteRead should be raised when the server sends an incomplete chunked response
939+
exc_types = (httplib.IncompleteRead, NetworkError)
940+
with pytest.raises(exc_types):
941+
writer._encoder.put([Span("foobar")])
942+
writer.flush_queue(raise_exc=True)
943+
944+
902945
@pytest.mark.parametrize("writer_class", (AgentWriter, NativeWriter))
903946
def test_flush_connection_uds(endpoint_uds_server, writer_class):
904947
writer = writer_class("unix://%s" % endpoint_uds_server.server_address)

0 commit comments

Comments
 (0)