Repository navigation
Recover from a lost UDP reply in seconds instead of minutes (#77, #78, #79, #93) - #140
Conversation
A dropped BMC reply used to cost the whole 300 s per-message timeout and an empty result. Four defects in the transport path made every retry pointless and every wait unbounded: - MessageListener kept the "Message timed out" error of the previous try, so a retry rethrew it at once instead of waiting for the resent message (#78). The listener now resets its outcome per try and waits on a monitor instead of polling. - MessageQueue kept a timed-out message for another timeout period and its tag reservation was inverted (#93). A timed-out message now leaves the queue at once and is reported once; a listener that throws no longer kills the timer thread, and the wait for a free queue slot is bounded by the message timeout. - Connection.waitForResponse counted sleeps instead of elapsed time and swallowed interruption (#79). It now waits on a wall-clock deadline, and an interrupt rolls the state machine back and propagates, so Future.cancel(true) from IpmiClient really stops the worker. The retry loops of the connectors no longer retry an interrupted step. The UDP receiver and the timers are daemon threads, and AbstractIpmiRunner .close() tolerates a session that never got a connector. - The default per-message timeout is 5 s instead of 300 s, and IpmiClient caps it by the overall timeout of the call (#77). IpmiConnector exposes getTimeout(handle). Verified on a GIGABYTE and a Lenovo BMC: a dropped Get SDR reply is now retried after 5 s and the sensor walk completes in about 9 s instead of timing out at 120 s; an unreachable host fails with "Command timed out" after 20 s, and the JVM exits without System.exit(). Fixes #77, fixes #78, fixes #79, fixes #93. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc485ac4c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (IllegalArgumentException | InterruptedException e) { | ||
| throw e; |
There was a problem hiding this comment.
When a thread using the synchronous low-level IpmiConnector.sendMessage is interrupted inside waitForAnswer, this new branch immediately rethrows, but sendMessage() unregisters its MessageListener only after a normal return. The listener therefore remains in IpmiAsyncConnector; repeated cancellations leak listeners, and every later response must traverse these stale registrations. Move listener removal into a finally block.
Useful? React with 👍 / 👎.
| pingPeriod=30000 | ||
| #Time in ms after which a message times out. | ||
| timeout=300000 | ||
| timeout=5000 |
There was a problem hiding this comment.
This changes the library-wide per-message timeout from five minutes to five seconds and accompanies new retry and cancellation semantics, but the README's 1.2.03 upgrade summary remains unchanged and omits this major user-visible behavior change. Update that summary so users relying on the previous default are warned.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
1 participant
Problem
A single dropped UDP reply from the BMC stalled an
IpmiClientcall for the 300 s per-message timeout and ended with a bareTimeoutExceptionand nothing collected. On the GIGABYTE test BMC this happens in roughly one run out of three.Four transport defects conspired:
MessageListenerkept the previous try'sMessage timed outerror, so every retry rethrew it immediately instead of waiting for the resent messageConnection.waitForResponsecounted 1 ms sleeps instead of elapsed time and swallowedInterruptedException; the receiver thread and timers were non-daemon;close()threw NPE when the connector was never createdclose()IpmiClientcaps it by the overall timeout;IpmiConnector.getTimeout(handle)addedBehaviour changes
ExecutionExceptionwrappingConnectionException: Command timed outafter about 20 s, where it used to throwTimeoutExceptionat the overall timeout.System.exit()is no longer needed at the end of command-line programs.QueueElement.isTimedOut(),makeTimedOut()andrefreshTimestamp()are removed (dead with the new queue behaviour). Listed inupgrading.md.Verification
mvn verifyon JDK 17: 41 tests, 0 checkstyle / PMD / CPD / SpotBugs findings.MessageListenerTest(stale error, interrupt),MessageQueueTest(removal on first timeout, timer survives a throwing listener),ConnectionTest(wall-clock timeout, interrupt rollback, daemon threads).Command timed outafter 20.4 s, JVM exited on its own. Overall timeout 5 s:TimeoutExceptionat 5.05 s, JVM exited at 5.25 s.Not in this PR (next one, same code area): #109 (state machine stuck after an authentication failure, credentials sent 4 times) and #132 (cleanup after the overall timeout runs alongside the worker, Close Session unconfirmed).
Fixes #77, fixes #78, fixes #79, fixes #93.
🤖 Generated with Claude Code