fix(q): bound poll-attached sync sends by the .q.connect timeout - #8
Open
belowzeroff wants to merge 1 commit into
Open
belowzeroff wants to merge 1 commit into
belowzeroff wants to merge 1 commit into
Conversation
.q.connect's timeout_ms is documented as a connect plus send/recv timeout, and q_connect applies it to the socket as SO_RCVTIMEO. That only bounds the blocking path: once .q.connect attaches the fd to the host's event loop (2.1.0), q_conn_send waits for the RESPONSE with ray_sock_wait_readable_intr(fd, -1), and poll() does not consult the socket's recv timeout. A peer that accepted the request and never answered parked the whole event loop for good — .q.send on a rayforce server never returned, and the timeout the caller had configured was silently ignored. q_conn_attach now reads the fd's SO_RCVTIMEO before switching it to non-blocking and keeps it as the connection's round-trip budget; q_conn_send turns it into a deadline and waits in bounded slices. When the deadline passes the connection is deregistered — its RESPONSE may still arrive and, left open, the next sync send on the handle would take it as its own reply — and a `timeout` error is returned. Handles without a budget behave as before. The blocking path already failed on an expired SO_RCVTIMEO, but .q.send reported it as a `send` error; it is now `timeout` on both paths. Tests: the exchange selftest attaches one end of a socketpair with a 200 ms recv timeout to a poll and sends into a peer that never answers, checking for the timeout error and the closed handle under a SIGALRM watchdog; 06_errors.rfl has q itself sleep past a 300 ms budget, in both the blocking and the --poll run.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
How it looks from the user's side
.q.connecttakes atimeout_msthat is documented as the connect and send/recv timeout. On a rayforce server (anything with an event loop —rayforce -p, the REPL), it bounded nothing after the connect:A q peer that accepted the request and did not answer — busy, wedged, or a non-q service that happened to complete the handshake — parked the server's whole event loop for good: no IPC clients served, no timers fired, no way out but a kill. The configured timeout was silently ignored, and the same script run without an event loop (the
.rfltest driver, a binding using only the blocking client) did time out as documented, so the hang looked like a server bug rather than a client one.Root cause
q_connectappliestimeout_msto the socket asSO_RCVTIMEO, which the kernel enforces per blockingrecv— the blocking path. Since 2.1.0.q.connectattaches the fd to the host's poll and.q.sendgoes throughq_conn_send, which waits for the RESPONSE withpoll()with an infinite timeout does not consult the socket's recv timeout, and the fd is non-blocking by then anyway. Nothing on that path knew the budget existed.Fix
q_conn_attachreads the fd'sSO_RCVTIMEObefore switching it to non-blocking and keeps it as the connection's round-trip budget (0 = none), so bindings attaching aq_connectfd inherit the timeout with no API change.q_conn_sendturns the budget into a deadline and waits in bounded slices. When it passes, the connection is deregistered — the request is in flight and its RESPONSE may still land; left open, the next sync send on the handle would claim it as its own reply — and atimeouterror is returned. The handle is then closed, like a peer that went away.SO_RCVTIMEObut.q.sendreported it as asenderror; it istimeouton both paths now, matching.q.connect's own timeout error.Tests
test/driver.cexchange selftest: one end of a socketpair with a 200 ms recv timeout is attached to a poll and sent into a peer that never answers; asserts thetimeouterror and that the handle is gone, under aSIGALRMwatchdog so a regression fails fast instead of hanging CI. Hangs on the previous code.test/rfl/client/06_errors.rfl: q itself is the slow peer (system "sleep 2"against a 300 ms budget) in both the blocking and the--pollrun, plus a fresh connection with an adequate budget still round-trips.make testagainst currentdevcore with a local q 4.x: all legs green (see log in the PR checks).