-
Notifications
You must be signed in to change notification settings - Fork 555
Integrate async transport with SDK #4615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: srothh/async-transport
Are you sure you want to change the base?
Integrate async transport with SDK #4615
Conversation
❌ 54 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
e5ea8e9
to
57ea658
Compare
caedf99
to
effb1a0
Compare
Add a new experimental option that uses AsyncHttpTransport and integrates it with synchronous client interfaces. GH-4601
Fix type issues in the Async Transport. GH-4601
Add event loop checks to client flush/close in async, in order to ensure silent failing instead of runtime error propagation/crashing. GH-4601
Moved the logic of the done callback directly into the async task to provide a cleaner and simpler control flow. GH-4601
effb1a0
to
425abaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Async Context Manager Fails to Await Cleanup
When using AsyncHttpTransport
, the synchronous __exit__
method calls self.close()
but does not await the returned asyncio.Task
. This prevents proper asynchronous cleanup (e.g., flushing events, closing transport) when the client is used as a context manager, potentially leading to lost events or resource leaks.
sentry_sdk/client.py#L1011-L1013
sentry-python/sentry_sdk/client.py
Lines 1011 to 1013 in 425abaf
def __exit__(self, exc_type: Any, exc_value: Any, tb: Any) -> None: | |
self.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Async Cleanup Fails in Sync Context
The __exit__
method calls self.close()
but does not await the asyncio.Task
returned when AsyncHttpTransport
is used. Since __exit__
is synchronous, this prevents the async flush and cleanup operations from completing, leading to potential data loss or resource leaks when the client is used as a context manager.
sentry_sdk/client.py#L1011-L1014
sentry-python/sentry_sdk/client.py
Lines 1011 to 1014 in 6bdc1ff
def __exit__(self, exc_type: Any, exc_value: Any, tb: Any) -> None: | |
self.close() | |
sentry_sdk/client.py
Outdated
|
||
if self.log_batcher is not None: | ||
self.log_batcher.flush() | ||
|
||
self.transport.flush(timeout=timeout, callback=callback) | ||
self.transport.flush(timeout=timeout, callback=callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Transport Flush Fails When Log Batcher Absent
In the synchronous transport path of the Client.flush
method, the self.transport.flush()
call is incorrectly indented. It is nested inside the if self.log_batcher is not None:
block, preventing the transport from flushing when log_batcher
is None
. This can lead to event loss, as the transport should always be flushed regardless of log_batcher
's presence.
Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Async Transport Cleanup Fails in Sync Context
The __exit__
method calls self.close()
but does not await the asyncio.Task
returned when AsyncHttpTransport
is in use. As __exit__
is synchronous, it cannot await this task, preventing proper flushing and cleanup of async transport resources when the client is used as a context manager. This leads to incomplete shutdown and potential loss of events.
sentry_sdk/client.py#L1011-L1013
sentry-python/sentry_sdk/client.py
Lines 1011 to 1013 in 67afe4d
def __exit__(self, exc_type: Any, exc_value: Any, tb: Any) -> None: | |
self.close() |
loop.close = _patched_close # type: ignore | ||
loop._sentry_flush_patched = True # type: ignore | ||
|
||
_patch_loop_close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Asyncio Loop Shutdown Issues
The _patch_loop_close()
function, intended to flush pending events before asyncio loop shutdown, has two issues. First, it often fails to patch the event loop because asyncio.get_running_loop()
raises a RuntimeError
when called during SDK initialization (before an event loop is running), preventing the patch from being applied in common scenarios. Second, even if patched, the _patched_close
function calls loop.run_until_complete(_flush())
from within loop.close()
. This is problematic as run_until_complete()
expects a non-running loop, while loop.close()
may be in an intermediate shutdown state, potentially leading to deadlocks, exceptions, or an inconsistent loop state. Additionally, _flush()
creates new tasks on a shutting-down loop, which may not execute reliably.
Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First issue: If init is called outside of an event loop, the transport falls back to the synchronous version anyway, so this should not be a problem.
Second issue: This is the same way the python discussion thread as well as asyncio-atexit handle it, so it seems to be the standard way of circumventing this issue. From my quick manual test, it also did not lead to problems.
I think this is not a problem. If this should work, I think it would need to implement a seperate async context manager. |
Integrate the async transport with the rest of the SDK. Provide a new experimental option
transport_async
that enables the async transport if an event loop is running. Otherwise, fall back to the sync transport.Furthermore, adapt the client to work with the async transport. To this end, flush and close were changed to be non blocking and awaitable in an async context to avoid deadlocks, however close enforces a completed flush before shutdown. As there are to my knowledge no background threads running flush/close, these methods are currently not thread-safe/loop-aware for async, which can be changed if necessary.
Atexit issue: The atexit integration used by the SDK runs after the event loop has already closed if asyncio.run() is used. This makes it impossible for the async flush to happen, as atexit calls client.close(), but a loop is no longer present. I attempted to apply this fix by patching the loop close in the asyncio integration, but I am unsure if I did it correctly/put it in the correct spot. From my SDK test however, it seems to fix the flush issue. Note also that this will apparently be patched in Python 3.14, as per the discussion in the linked thread.
As a final note, I added event loop checking. Whenever the event loop is used, the transport/client catch RuntimeErrors, which would be thrown in case the event loop was already shut down. I am not sure if this is a case we need to consider, but I added it for now because I did not want the transport to potentially throw RuntimeError if the event loop is shutdown during a program. If this should be left out currently for simplicity, I can remove it again.
Do I also need to add the httpcore[asyncio] dependency to requirements-testing ?
GH-4601