From 0fdccde531ba33c09f6b0ed41e6e7c32d8e93d98 Mon Sep 17 00:00:00 2001 From: Daniel McCoy Stephenson Date: Thu, 24 Sep 2026 01:05:40 -0600 Subject: [PATCH] Keep report() from raising on a non-string name and the sender alive on a scheme-less URL report() checked the name before its try, so report(123) raised AttributeError into the host program. _send() built the Request before its try, so a base_url with no scheme killed the sender thread with a traceback on stderr. Both now fall inside the existing try and are logged at DEBUG like every other dropped report. Closes #2 Closes #3 Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_trace_client.py | 22 ++++++++++++++++++++++ trace_client/trace_client.py | 20 ++++++++++++-------- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/tests/test_trace_client.py b/tests/test_trace_client.py index 483f4a6..cb04572 100644 --- a/tests/test_trace_client.py +++ b/tests/test_trace_client.py @@ -224,6 +224,28 @@ def test_report_ignores_a_blank_name(self): client.close() self.assertFalse(self.capture.arrived.wait(0.3)) + def test_report_does_not_raise_for_a_name_that_is_not_a_string(self): + client = TraceClient(self.base_url, "MyGame", key="k") + client.report(123) # must not raise + client.close() + self.assertFalse(self.capture.arrived.wait(0.3), "a report that could not be built is dropped") + self.assertTrue(any("could not queue 123" in r.getMessage() for r in self.log), + [r.getMessage() for r in self.log]) + self.assertTrue(all(r.levelno == logging.DEBUG for r in self.log)) + + def test_a_base_url_without_a_scheme_is_logged_not_a_dead_thread(self): + crashes = [] + with mock.patch.object(threading, "excepthook", crashes.append): + client = TraceClient("trace.example.org", "MyGame", key="k") + client.report("startup") + client.report("shutdown") + client.close() + self.assertEqual([], crashes, "the sender thread must not die with a traceback on stderr") + failures = [r for r in self.log if "could not deliver" in r.getMessage()] + self.assertEqual(2, len(failures), "each report is logged and the thread keeps going: %s" + % [r.getMessage() for r in self.log]) + self.assertTrue(all(r.levelno == logging.DEBUG for r in self.log)) + def test_constructor_rejects_a_missing_base_url_or_application(self): for base_url, application in ((None, "MyGame"), (" ", "MyGame"), ("http://x", None), ("http://x", "")): with self.assertRaises(ValueError): diff --git a/trace_client/trace_client.py b/trace_client/trace_client.py index 6b85173..be6d22d 100644 --- a/trace_client/trace_client.py +++ b/trace_client/trace_client.py @@ -134,9 +134,11 @@ def report(self, name: str, value: Optional[float] = None, tags: Optional[Mapping[str, str]] = None) -> None: """Report that ``name`` happened, with an optional numeric value and optional string tags. Returns immediately; see the class docstring.""" - if self._queue is None or not name or not name.strip(): + if self._queue is None: return try: + if not name or not name.strip(): + return body = _json(self._application, name, value, tags) self._queue.put_nowait(body) except queue.Full: @@ -181,14 +183,16 @@ def _drain(self) -> None: self._send(body) def _send(self, body: bytes) -> None: - request = urllib.request.Request( - self._endpoint, data=body, method="POST", - headers={ - "Content-Type": "application/json; charset=utf-8", - "Authorization": "Bearer " + self._key, - "User-Agent": "trace-client-python/%s (%s)" % (__version__, self._application), - }) try: + # Built inside the try: a base URL without a scheme fails here, and + # an uncaught error would kill the sender thread with a traceback. + request = urllib.request.Request( + self._endpoint, data=body, method="POST", + headers={ + "Content-Type": "application/json; charset=utf-8", + "Authorization": "Bearer " + self._key, + "User-Agent": "trace-client-python/%s (%s)" % (__version__, self._application), + }) with urllib.request.urlopen(request, timeout=self.TIMEOUT_SECONDS) as response: status = response.status response.read()