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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 3 commits into from

Conversation

NoahStapp
Copy link
Contributor

…ady been execeeded

@ShaneHarvey
Copy link
Member

ShaneHarvey commented May 1, 2025 •
edited
Loading

On second thought, I intentionally added the non-blocking read to avoid spurious reconnects on SDAM connections when using the driver in FaaS services that pause/resume the process, see https://jira.mongodb.org/browse/PYTHON-3186.

I'm not sure we can accept this change. Although, if the test_sigstop_sigcont regression test still passes, perhaps we can?

@sleepyStick
Copy link
Contributor

On second thought, I intentionally added the non-blocking read to avoid spurious reconnects on SDAM connections when using the driver in FaaS services that pause/resume the process, see jira.mongodb.org/browse/PYTHON-3186.

I'm not sure we can accept this change. Although, if the test_sigstop_sigcont regression test still passes, perhaps we can?

Looks like that regression test isn't passing...
https://spruce.mongodb.com/task/mongo_python_driver_test_macos_arm64_test_standard_v6.0_python3.13_sync_auth_ssl_sharded_cluster_patch_0ec57781d14160531887f61e4edf083acbef7686_6813d280e5d2f8000770ece5_25_05_01_19_58_57?execution=0&sortBy=STATUS&sortDir=ASC

@blink1073
Copy link
Member

Can we not use the FaaS detection logic and preserve the current behavior in that case?

@NoahStapp
Copy link
Contributor Author

Since test_sigstop_sigcont was added specifically for the FaaS fix, can we limit it to only run on FaaS test variants?

@blink1073
Copy link
Member

Since test_sigstop_sigcont was added specifically for the FaaS fix, can we limit it to only run on FaaS test variants?

My vote is yes

ShaneHarvey
ShaneHarvey reviewed May 5, 2025
View reviewed changes
test/test_client.py
@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneHarvey ShaneHarvey ShaneHarvey left review comments

@sleepyStick sleepyStick sleepyStick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NoahStapp @ShaneHarvey @sleepyStick @blink1073