ref(transport): Add shared sync/async transport superclass and create a sync http subclass #4572
+610
−65
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌ 17 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
53b92f2 to
3607d44
Compare
antonpirker
reviewed
Jul 24, 2025
…ted a sync transport HTTP subclass Moved shared sync/async logic into a new superclass (HttpTransportCore), and moved sync transport specific code into a new subclass(BaseSyncHttpTransport), from which the current transport implementations inherit Fixes GH-4568
Removed an unnecessary TODO message and reverted a class name change for BaseHTTPTransport. GH-4568
Adds test coverage for the error handling path when HTTP requests return error status codes. GH-4568
Restore comments accidentally removed during a previous commit.
Refactored class names such that BaseHttpTransport now has the same functionality as before the hierarchy refactor GH-4568
Add a new flush_async method in the Transport ABC. This is needed for the async transport, as calling it from the client while preserving execution order in close will require flush to be a coroutine, not a function. GH-4568
Move flush_async down to the specific async transport subclass. This makes more sense anyway, as this will only be required by the async transport. If more async transports are expected, another shared superclass can be created. GH-4568
Add necessary type annotations to the core HttpTransport to accomodate for async transport. GH-4568
…nd cleaner return paths GH-4568
062ab5b to
4e56e5c
Compare
|
okay, so now that we have 2 base/core classes, it's a bit more confusing than before what the contracts of each entity actually are. I want us to define more clearly and explicitly what is
but I think it's clearer if we merge all the changes in first and then do some shuffling. We will do that with mixin classes. |
…orker (#4580) Add a new abstract base class for the background worker implementations in the transport. This allows for implementation of the upcoming async task-based worker in addition to the current thread based worker used in the sync context. Additionally, add a factory method in the HttpTransportCore shared class, to allow the worker methods to live higher up in the class hierarchy regardless of specific implementation. GH-4578
Add a new implementation of the transport background worker based on an async task. This worker mostly mirrors the same functionality as the thread-based worker, with the exception that it exposes a non-blocking async flush (which can be awaited from an async context). Furthermore, the worker itself is not thread-safe and should be called using [run_coroutine_threadsafe](https://docs.python.org/3/library/asyncio-task.html#asyncio.run_coroutine_threadsafe) or similar when called from another thread (this is fixed and handled by the transport). I have kept the fork check from the threaded worker, but I am not sure if it is necessary as forking in an async application would also break the event loop. GH-4581 --------- Co-authored-by: Neel Shah <neel.shah@sentry.io>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add async implementation of the abstract Transport class. This transport utilizes the async task worker as well as the httpcore async functionality. Thread Safety: As capture_envelope is registered by the client as a callback for several background threads in the sdk, which are not running the event loop, capture_envelope in the transport is made to be thread safe and allow for execution on the event loop from other threads. The same is currently not the case for flush, as there does not seem to be a usage from background threads, however if necessary, it can also be added. HTTP2 support: Currently not activated, but from the looks of the [httpcore docs](https://www.encode.io/httpcore/http2/) it should be as simple as setting the http2 in the init of the pool to true. This likely makes sense to support, as HTTP2 shows great performance improvements with concurrent requests. Kill: The kill method is sync, but the pool needs to be closed asynchronously. Currently, this is done by launching a task. However, the task cannot be awaited in sync code without deadlocking, therefore kill followed by an immediate loop shutdown could technically lead to resource leakage. Therefore, I decided to make kill optionally return the async task, so it can be awaited if called from an async context. Note also that parts of the code are very similar to the HTTP2 integration, as they both use the httpcore library. Maybe in a later PR there could be a shared superclass to avoid code duplication? GH-4582 --------- Co-authored-by: Neel Shah <neel.shah@sentry.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moved shared sync/async logic into superclass, and moved sync transport specific code into a new subclass(BaseSyncHttpTransport), from which the current transport implementations inherit. Note that currently the threaded worker is still being created in the superclass. In a next step, I want to add an abstract worker class for both the threaded and async task worker, however I wanted to keep this PR atomic and only re-implement the sync transport with the new hierarchy.
Fixes GH-4568