PYTHON-4725 Async client should use tasks for SDAM instead of threads #1896
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
blink1073
reviewed
Oct 4, 2024
pymongo/asynchronous/monitor.py
Outdated
| if _IS_SYNC: | ||
| _shutdown_resources() | ||
| else: | ||
| asyncio.run(_shutdown_resources()) |
There was a problem hiding this comment.
This won't work because there is already an event loop running, we need something like https://github.com/minrk/asyncio-atexit
There was a problem hiding this comment.
yeah, something was funky HAHA thanks for showing me a fix!
There was a problem hiding this comment.
There was a problem hiding this comment.
would i be allowed to import this and use the package? or should i just take the code that we need and put in on our own code base? thoughts?
There was a problem hiding this comment.
Do we need this at all? What happens if we do nothing for async clients at exit?
Do our SDAM tasks prevent the loop from exiting?
We document that an AsyncMongoClient must always be closed() so I don't believe we need to do anything. If the app leaves the client open, that's a bug.
There was a problem hiding this comment.
I took it out of async and it seems to be fine so far.
…routine in sync verison
ShaneHarvey
reviewed
Oct 9, 2024
| @@ -28,6 +28,99 @@ | |||
| _IS_SYNC = True | |||
|
|
|||
|
|
|||
| class SyncPeriodicExecutor: | |||
There was a problem hiding this comment.
We should organize the code such that we don't sync this code since it will never be used.
There was a problem hiding this comment.
As in move this file out of the async / sync folders right? If so, that makes sense to me :)
There was a problem hiding this comment.
Yep. We can have one file that has both.
ShaneHarvey
reviewed
Oct 9, 2024
test/asynchronous/__init__.py
Outdated
| return | ||
| global async_client_context | ||
| await async_teardown() | ||
| async_client_context = AsyncClientContext() |
There was a problem hiding this comment.
This doesn't work because the old client_context was already imported by other files so this change won't be reflected there. For example:
>>> import pymongo
>>> from pymongo import ASCENDING
>>> ASCENDING
1
>>> pymongo.ASCENDING = 2
>>> ASCENDING
1
>>> pymongo.ASCENDING
2Instead we need to mutate the existing ClientContext
There was a problem hiding this comment.
Ahhh that makes sense, thanks!
blink1073
reviewed
Oct 10, 2024
|
One issue: A test emits an error from the _process_kill_cursors task (I'm not sure if this is expected or not): Another issue a test teardown times out after blocking for 25 minutes: I don't see any time outs like this happening on mainline so I think it's related to the changes here. Both happened on execution 1 of this task: |
ShaneHarvey
approved these changes
Oct 12, 2024
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.
I created an
AsyncPeriodicExecutorclass because it differs enough from the regularPeriodicExecutorclass -> this resulted in moving upperiodic_executor.pyup to the top level.In the process of working on this ticket, I noticed some bugs in the tests (async tests using
wait_untilinstead ofasync_wait_until) that are now appearing since background tasks are running in the event loop instead of on threads.