-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5021 - Fix usages of getaddrinfo to be non-blocking #2059
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
Merged
Merged
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
f7bb39e
PYTHON-5021 - Fix usages of getaddrinfo to be non-blocking
NoahStapp 882c0db
Merge branch 'master' into PYTHON-5021
NoahStapp f6c0136
Use get_running_loop instead of get_event_loop
NoahStapp 076a014
Use our own executor
NoahStapp 2821566
Fix import of _PYMONGO_EXECUTOR
NoahStapp 26fe6e1
Merge branch 'master' into PYTHON-5021
NoahStapp 297bf9c
getaddrinfo helper method
NoahStapp 39ade36
cleanup
NoahStapp 5e3bc65
Use run_in_executor for getaddrinfo
NoahStapp 85e59fd
Revert back to using the default executor
NoahStapp abe6b24
getaddrinfo -> _getaddrinfo
NoahStapp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Copyright 2024-present MongoDB, Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
"""A separate ThreadPoolExecutor instance used internally to avoid competing for resources with the default asyncio ThreadPoolExecutor | ||
that user code will use.""" | ||
|
||
from __future__ import annotations | ||
|
||
from concurrent.futures import ThreadPoolExecutor | ||
|
||
_PYMONGO_EXECUTOR = ThreadPoolExecutor(thread_name_prefix="PYMONGO_EXECUTOR-") | ||
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,10 @@ | |
"""Miscellaneous pieces that need to be synchronized.""" | ||
from __future__ import annotations | ||
|
||
import asyncio | ||
import builtins | ||
import functools | ||
import socket | ||
import sys | ||
from typing import ( | ||
Any, | ||
|
@@ -24,6 +27,7 @@ | |
cast, | ||
) | ||
|
||
from pymongo._asyncio_executor import _PYMONGO_EXECUTOR | ||
from pymongo.errors import ( | ||
OperationFailure, | ||
) | ||
|
@@ -68,6 +72,26 @@ def inner(*args: Any, **kwargs: Any) -> Any: | |
return cast(F, inner) | ||
|
||
|
||
def getaddrinfo( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: let's rename this |
||
host: Any, port: Any, **kwargs: Any | ||
) -> list[ | ||
tuple[ | ||
socket.AddressFamily, | ||
socket.SocketKind, | ||
int, | ||
str, | ||
tuple[str, int] | tuple[str, int, int, int], | ||
] | ||
]: | ||
if not _IS_SYNC: | ||
loop = asyncio.get_running_loop() | ||
return loop.run_in_executor( # type: ignore[return-value] | ||
_PYMONGO_EXECUTOR, functools.partial(socket.getaddrinfo, host, port, **kwargs) | ||
) | ||
else: | ||
return socket.getaddrinfo(host, port, **kwargs) | ||
|
||
|
||
if sys.version_info >= (3, 10): | ||
next = builtins.next | ||
iter = builtins.iter | ||
|
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
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
Oops, something went wrong.
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.
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.
I'm not sure I like this approach because we now have a thread pool that hangs around forever even after all clients have been closed.
My other comment was more around adding guidance for potential errors, not for changing our implementation. Like something that says "if your app runs into "XXX" error consider this may mean your app's default loop executor is under provisioned. Consider increasing the size of this thread pool or ..."
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.
It could be difficult to distinguish when this issue occurs, having a separate thread pool for our internal use will help mitigate how common it is. An extra thread pool instance shouldn't be expensive to have a reference to for the lifetime of the application.
Uh oh!
There was an error while loading. Please reload this page.
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.
Personally I prefer we go with the loop.getaddrinfo approach because it avoids adding the complexity of managing our own thread pool. It's not really kosher to leave a threadpool open even if the threads are "idle". The limitation in loop.getaddrinfo is also implementation detail that could be fixed at any point (even in a python bugfix release).
I expect it will be clear when this issue occurs because a timeout error caused by threadpool starvation looks different than a real DNS timeout error. It should be simple to add an example to our docs by:
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.
I don't see much complexity in managing our own thread pool, but I totally understand the desire to not have an extra pool lying around. I'll revert back to using
loop.getaddrinfo()
once I have a good example for our docs.Uh oh!
There was an error while loading. Please reload this page.
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.
After investigating, I believe the docs are slightly misleading: what actually happens when the executor pool is fully saturated is any
loop.getaddrinfo()
calls block until a thread is freed up for use. There's no timeout mechanism inherent to the executor pool. We could add our own timeout to everyloop.run_in_executor()
call to prevent users from accidentally blocking the driver forever if they saturate the default executor permanently, but then we would cause timeouts to occur whenever the response is too slow.If we don't add any timeouts to those calls, users will experience slowdowns whenever they perform a driver operation while the default executor pool is fully saturated. That's preferable to spurious timeouts in my opinion, especially when the user's own code is what determines the frequency of the timeouts.
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.
Thanks for investigating, I agree with that. The cpython ticket referencing anyio so that could explain the difference.