fix: poll POLLOUT in loop instead of aborting after single 300ms miss#69816
fix: poll POLLOUT in loop instead of aborting after single 300ms miss#69816mangodxd wants to merge 2 commits into
Conversation
In RequestClient._send_recv, a single 300ms socket.poll(POLLOUT) miss was treated as an immediate SaltReqTimeoutError, ignoring the caller's configured request timeout (return_retry_timer / request_channel_timeout). Mirror the POLLIN loop pattern already used later in the same method: poll in a loop, only abort when the future completes (timeout fired). Fixes saltstack#69802
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey. There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
twangboy
left a comment
twangboy
left a comment
There was a problem hiding this comment.
This needs a changelog and a test as well.
| if not future.done(): | ||
| future.set_exception( | ||
| SaltReqTimeoutError("Socket not ready for sending") | ||
| ) |
There was a problem hiding this comment.
You can safely remove lines 2168 through 2171. They are never reacheable. If the future is already done, it already holds the appropriate SaltReqTimeoutError or cancellation state from the overarching timeout handler, and attempting to mutate it further isn't necessary. The reconnect logic (lines 2172-2173) is the only part of that fallback block that actually needs to run.
…etry test - Remove unreachable lines 2168-2171 (future already done per review) - Add changelog/69816.fixed.md - Add test_request_client_pollout_retries_on_transient_failure verifying that consecutive POLLOUT misses are retried
|
Thanks for the review @twangboy. I've addressed all three items:
Updates pushed to the branch. |
twangboy
left a comment
twangboy
left a comment
There was a problem hiding this comment.
Please rebase this PR on the oldest supported branch where this bug exists. Probably 3006.x.
|
Thanks for reviewing! The changelog and test are actually already included in this PR:
Could you take another look? Happy to make any additional changes if needed. |
2 participants
What does this PR do?
Fixes a hard-coded 300ms POLLOUT timeout in RequestClient._send_recv that caused premature SaltReqTimeoutError even when the caller configured a larger request timeout.
What issues does this PR fix?
Fixes #69802
Previous Behavior
A single 300ms socket.poll(zmq.POLLOUT) miss immediately raised SaltReqTimeoutError("Socket not ready for sending"), ignoring the caller's configured
eturn_retry_timer /
equest_channel_timeout.
New Behavior
Polls for POLLOUT in a loop (mirroring the existing POLLIN pattern), only aborting when the future completes via the configured timeout or cancellation.
How to test
`python
Existing tests should continue to pass:
tests/pytests/unit/transport/test_zeromq.py::test_client_send_recv_on_cancelled_error
tests/pytests/unit/transport/test_zeromq.py::test_client_send_recv_no_double_set_exception_after_timeout
`
Commits