PYTHON-5369 - Re-raise socket.timeout errors if the deadline has alre…#2326
PYTHON-5369 - Re-raise socket.timeout errors if the deadline has alre…#2326NoahStapp merged 10 commits intomongodb:masterfrom
Conversation
…ady been execeeded
|
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? |
Looks like that regression test isn't passing... |
|
Can we not use the FaaS detection logic and preserve the current behavior in that case? |
|
Since |
My vote is yes |
test/test_client.py
Outdated
| "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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Perhaps instead we can force the metadata to be faas through monkeypatching
test/asynchronous/test_client.py
Outdated
| ) | ||
| 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 |
ShaneHarvey
left a comment
ShaneHarvey
left a comment
There was a problem hiding this comment.
Did we determine that this change actually fixes the test flakes?
I'm running some patches right now to confirm, but so far it appears to fix the flakes on non-PyPy variants. PyPy still has this issue, so I propose we skip |
I have not seen any failures of these tests flaking on sync + CPython with this fix. |
| or not _is_faas() | ||
| and deadline is not None | ||
| and deadline - time.monotonic() < 0 | ||
| ): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Are the flaky tests still fixed after the is_sdam change?
There was a problem hiding this comment.
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.
4 participants
…ady been execeeded