Skip to content

PYTHON-5369 - Re-raise socket.timeout errors if the deadline has alre… #2326

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.

Sign up for GitHub

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 10 commits into from
May 12, 2025
Merged

PYTHON-5369 - Re-raise socket.timeout errors if the deadline has alre… #2326

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7a15f04
PYTHON-5369 - Re-raise socket.timeout errors if the deadline has alre…
NoahStapp May 1, 2025
59e1897
Try detecting faas
NoahStapp May 2, 2025
7765b7d
Only run test_sigstop_sigint on FaaS
NoahStapp May 2, 2025
55324d4
Mock running on FaaS
NoahStapp May 6, 2025
4005821
Remove unused import
NoahStapp May 8, 2025
d851d85
Rename handshake to is_sdamm and use to detect when to re-raise netwo…
NoahStapp May 8, 2025
e775c65
Fix default is_sdam setting
NoahStapp May 8, 2025
9136b56
Merge branch 'master' into PYTHON-5369
NoahStapp May 8, 2025
968fc56
Fix test_timeout_configuration
NoahStapp May 8, 2025
e5ec63a
Merge branch 'master' into PYTHON-5369
NoahStapp May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pymongo/network_layer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from pymongo.compression_support import decompress
from pymongo.errors import ProtocolError, _OperationCancelled
from pymongo.message import _UNPACK_REPLY, _OpMsg, _OpReply
from pymongo.pool_options import _is_faas
from pymongo.socket_checker import _errno_from_exception

try:
Expand Down Expand Up @@ -357,7 +358,12 @@ def receive_data(conn: Connection, length: int, deadline: Optional[float]) -> me
except socket.timeout:
if conn.cancel_context.cancelled:
raise _OperationCancelled("operation cancelled") from None
if _PYPY:
if (
_PYPY
or not _is_faas()
and deadline is not None
and deadline - time.monotonic() < 0
):
Copy link
Member

@ShaneHarvey ShaneHarvey May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want sigstop/sigcont to work the same regardless of what env variables are defined. What if we take an alternative approach here and replace not _is_faas() with not conn.is_sdam? IE only do the extra non-blocking read on SDAM connections.

Will that still fix the flaky tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the flaky tests still fixed after the is_sdam change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They appear to be, yes: https://spruce.mongodb.com/version/681e08d75668e00007df8859/tasks?page=0&sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC&statuses=failed&taskName=%5E%28%28%3F%21pypy%29.%29*%24&variant=%5E%28%28%3F%21pypy%29.%29*%24. Filtering out PyPy failures, none of the flaky tests that should be fixed by this change are present.

# We reached the true deadline.
raise
continue
Expand Down
3 changes: 2 additions & 1 deletion test/asynchronous/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
WriteConcernError,
)
from pymongo.monitoring import ServerHeartbeatListener, ServerHeartbeatStartedEvent
from pymongo.pool_options import _MAX_METADATA_SIZE, _METADATA, ENV_VAR_K8S, PoolOptions
from pymongo.pool_options import _MAX_METADATA_SIZE, _METADATA, ENV_VAR_K8S, PoolOptions, _is_faas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this import

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

from pymongo.read_preferences import ReadPreference
from pymongo.server_description import ServerDescription
from pymongo.server_selectors import readable_server_selector, writable_server_selector
Expand Down Expand Up @@ -2010,6 +2010,7 @@ async def test_srv_max_hosts_kwarg(self):
"loadBalanced clients do not run SDAM",
)
@unittest.skipIf(sys.platform == "win32", "Windows does not support SIGSTOP")
@unittest.skipUnless(_is_faas(), "Non-FaaS environments raise timeouts faster")
@async_client_context.require_sync
def test_sigstop_sigcont(self):
test_dir = os.path.dirname(os.path.realpath(__file__))
Expand Down
3 changes: 2 additions & 1 deletion test/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@
WriteConcernError,
)
from pymongo.monitoring import ServerHeartbeatListener, ServerHeartbeatStartedEvent
from pymongo.pool_options import _MAX_METADATA_SIZE, _METADATA, ENV_VAR_K8S, PoolOptions
from pymongo.pool_options import _MAX_METADATA_SIZE, _METADATA, ENV_VAR_K8S, PoolOptions, _is_faas
from pymongo.read_preferences import ReadPreference
from pymongo.server_description import ServerDescription
from pymongo.server_selectors import readable_server_selector, writable_server_selector
Expand Down Expand Up @@ -1967,6 +1967,7 @@ def test_srv_max_hosts_kwarg(self):
"loadBalanced clients do not run SDAM",
)
@unittest.skipIf(sys.platform == "win32", "Windows does not support SIGSTOP")
@unittest.skipUnless(_is_faas(), "Non-FaaS environments raise timeouts faster")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't skip this test like this because we don't run the test suite on faas platforms. We want this test to run locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "run locally", what exactly do you mean? As in we can't do this change because we need this test to run in our regular test suite?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps instead we can force the metadata to be faas through monkeypatching

@client_context.require_sync
def test_sigstop_sigcont(self):
test_dir = os.path.dirname(os.path.realpath(__file__))
Expand Down
Loading