Add transport worker factory and abstract base class for background worker #4580
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
❌ 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 |
The uncovered lines of code here are just the pass calls from the abstract class definition. |
srothh
force-pushed
the
srothh/transport-class-hierarchy
branch
from
53b92f2 to
3607d44
Compare
srothh
force-pushed
the
srothh/worker-class-hierarchy
branch
from
1261319 to
2896602
Compare
Add an abstract bass class for implementation of the background worker. This was done to provide a shared interface for the current implementation of a threaded worker in the sync context as well as the upcoming async task-based worker implementation. GH-4578
Add a new factory method instead of direct instatiation of the threaded background worker. This allows for easy extension to other types of workers, such as the upcoming task-based async worker. GH-4578
Add a new flush_async method to worker ABC. This is necessary because the async transport cannot use a synchronous blocking flush. GH-4578
Move the flush_async down to the concrete subclass to not break existing testing. This makes sense, as this will only really be needed by the async worker anyway and therefore is not shared logic. GH-4578
Coroutines have a return value, however the current function signature for the worker methods does not accomodate for this. Therefore, this signature was changed. GH-4578
srothh
force-pushed
the
srothh/worker-class-hierarchy
branch
from
2896602 to
268ea1a
Compare
antonpirker
approved these changes
Jul 24, 2025
| @property | ||
| @abstractmethod | ||
| def is_alive(self) -> bool: | ||
| pass |
There was a problem hiding this comment.
Please add docstrings to all methods in the new abstract worker class.
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.
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