Skip to content

Commit ff3e9a0

Browse files
BYKclaude
andcommitted
refactor: Address reviewer feedback — rename, simplify, remove tests
Per sentrivana's review: Source changes: - Rename ASYNC_TRANSPORT_ENABLED -> ASYNC_TRANSPORT_AVAILABLE - Rename _is_async_transport -> _has_async_transport on _Client - Use warnings.warn instead of logger.warning for close()/flush() with async transport - Restore keep_alive guard in _get_httpcore_pool_options (the original Http2Transport behavior was intentional, but the reviewer wants it guarded) - Add httpcore[asyncio] to tox.jinja for linters/mypy/common - Revert tox.ini to origin/master (auto-generated, needs regen) - Revert AGENTS.md lore section Test removals (~1050 lines): - Remove ALL test_async_worker_* tests (implementation details) - Remove ALL test_make_transport_* tests (implementation details) - Remove all test_handle_response_*, test_update_headers, test_prepare_envelope_* tests (implementation details or duplicates) - Remove test_close_async_awaits_kill_task (duplicate) - Remove test_handle_request_error_basic_coverage (too much mocking) - Remove test_async_transport_event_loop_closed_scenario (mocking-heavy) - Remove test_async_transport_get_header_value (no added benefit) - Remove misc implementation-detail async transport tests - Apply make_client() simplification suggestions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 025714e commit ff3e9a0

File tree

6 files changed

+58
-1116
lines changed

6 files changed

+58
-1116
lines changed

scripts/populate_tox/tox.jinja

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,17 @@ deps =
7878
7979
linters: -r requirements-linting.txt
8080
linters: werkzeug<2.3.0
81+
linters: httpcore[asyncio]
8182
8283
mypy: -r requirements-linting.txt
8384
mypy: werkzeug<2.3.0
85+
mypy: httpcore[asyncio]
8486
ruff: -r requirements-linting.txt
8587
8688
# === Common ===
8789
py3.8-common: hypothesis
8890
common: pytest-asyncio
91+
common: httpcore[asyncio]
8992
# See https://github.com/pytest-dev/pytest/issues/9621
9093
# and https://github.com/pytest-dev/pytest-forked/issues/67
9194
# for justification of the upper bound on pytest

sentry_sdk/client.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,7 @@ def get_integration(
10091009

10101010
return self.integrations.get(integration_name)
10111011

1012-
def _is_async_transport(self) -> bool:
1012+
def _has_async_transport(self) -> bool:
10131013
"""Check if the current transport is async."""
10141014
return isinstance(self.transport, AsyncHttpTransport)
10151015

@@ -1045,11 +1045,10 @@ def close(
10451045
semantics as :py:meth:`Client.flush`.
10461046
"""
10471047
if self.transport is not None:
1048-
if self._is_async_transport():
1049-
logger.warning(
1050-
"close() used with AsyncHttpTransport. "
1051-
"Prefer close_async() for graceful async shutdown. "
1052-
"Performing synchronous best-effort cleanup."
1048+
if self._has_async_transport():
1049+
warnings.warn(
1050+
"close() used with AsyncHttpTransport. Use close_async() instead.",
1051+
stacklevel=2,
10531052
)
10541053
self._flush_components()
10551054
else:
@@ -1068,7 +1067,7 @@ async def close_async(
10681067
semantics as :py:meth:`Client.flush_async`.
10691068
"""
10701069
if self.transport is not None:
1071-
if not self._is_async_transport():
1070+
if not self._has_async_transport():
10721071
logger.debug(
10731072
"close_async() used with non-async transport, aborting. Please use close() instead."
10741073
)
@@ -1093,9 +1092,10 @@ def flush(
10931092
:param callback: Is invoked with the number of pending events and the configured timeout.
10941093
"""
10951094
if self.transport is not None:
1096-
if self._is_async_transport():
1097-
logger.warning(
1098-
"flush() used with AsyncHttpTransport. Please use flush_async() instead."
1095+
if self._has_async_transport():
1096+
warnings.warn(
1097+
"flush() used with AsyncHttpTransport. Use flush_async() instead.",
1098+
stacklevel=2,
10991099
)
11001100
return
11011101
if timeout is None:
@@ -1117,7 +1117,7 @@ async def flush_async(
11171117
:param callback: Is invoked with the number of pending events and the configured timeout.
11181118
"""
11191119
if self.transport is not None:
1120-
if not self._is_async_transport():
1120+
if not self._has_async_transport():
11211121
logger.debug(
11221122
"flush_async() used with non-async transport, aborting. Please use flush() instead."
11231123
)

sentry_sdk/transport.py

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@
3131
try:
3232
import anyio # noqa: F401
3333

34-
ASYNC_TRANSPORT_ENABLED = httpcore is not None
34+
ASYNC_TRANSPORT_AVAILABLE = httpcore is not None
3535
except ImportError:
36-
ASYNC_TRANSPORT_ENABLED = False
36+
ASYNC_TRANSPORT_AVAILABLE = False
3737

3838
import urllib3
3939
import certifi
@@ -545,18 +545,22 @@ def _get_httpcore_pool_options(
545545
"retries": 3,
546546
}
547547

548-
socket_options = (
549-
self.options["socket_options"]
550-
if self.options["socket_options"] is not None
551-
else []
552-
)
548+
socket_options: "Optional[List[Tuple[int, int, int | bytes]]]" = None
549+
550+
if self.options["socket_options"] is not None:
551+
socket_options = self.options["socket_options"]
553552

554-
used_options = {(o[0], o[1]) for o in socket_options}
555-
for default_option in KEEP_ALIVE_SOCKET_OPTIONS:
556-
if (default_option[0], default_option[1]) not in used_options:
557-
socket_options.append(default_option)
553+
if self.options["keep_alive"]:
554+
if socket_options is None:
555+
socket_options = []
558556

559-
options["socket_options"] = socket_options
557+
used_options = {(o[0], o[1]) for o in socket_options}
558+
for default_option in KEEP_ALIVE_SOCKET_OPTIONS:
559+
if (default_option[0], default_option[1]) not in used_options:
560+
socket_options.append(default_option)
561+
562+
if socket_options is not None:
563+
options["socket_options"] = socket_options
560564

561565
ssl_context = ssl.create_default_context()
562566
ssl_context.load_verify_locations(
@@ -838,7 +842,7 @@ def _request(
838842

839843
class AsyncHttpTransport(HttpTransportCore):
840844
def __init__(self: "Self", options: "Dict[str, Any]") -> None:
841-
if not ASYNC_TRANSPORT_ENABLED:
845+
if not ASYNC_TRANSPORT_AVAILABLE:
842846
raise RuntimeError(
843847
"AsyncHttpTransport requires httpcore[asyncio]. "
844848
"Install it with: pip install sentry-sdk[asyncio]"
@@ -1120,7 +1124,7 @@ def make_transport(options: "Dict[str, Any]") -> "Optional[Transport]":
11201124
Http2Transport if use_http2_transport else HttpTransport
11211125
)
11221126

1123-
if use_async_transport and ASYNC_TRANSPORT_ENABLED:
1127+
if use_async_transport and ASYNC_TRANSPORT_AVAILABLE:
11241128
try:
11251129
asyncio.get_running_loop()
11261130
if async_integration:

tests/test_client.py

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1833,23 +1833,6 @@ async def mock_wait_flush(timeout, callback=None):
18331833
await client.close_async()
18341834

18351835

1836-
@skip_under_gevent
1837-
@pytest.mark.asyncio
1838-
@pytest.mark.skipif(not PY38, reason="Async client methods require Python 3.8+")
1839-
async def test_close_async_awaits_kill_task():
1840-
"""Test close_async() awaits the kill task returned by transport.kill()."""
1841-
client = Client(
1842-
"https://foo@sentry.io/123",
1843-
_experiments={"transport_async": True},
1844-
integrations=[AsyncioIntegration()],
1845-
)
1846-
assert isinstance(client.transport, AsyncHttpTransport)
1847-
1848-
# close_async should call kill() and await the returned task
1849-
await client.close_async(timeout=1.0)
1850-
assert client.transport is None
1851-
1852-
18531836
@skip_under_gevent
18541837
@pytest.mark.asyncio
18551838
@pytest.mark.skipif(not PY38, reason="Async client methods require Python 3.8+")

0 commit comments

Comments
 (0)