Repository navigation
Make a connection safe for its sending, receiving and keep-alive threads (#96, #89) - #149
Conversation
StateMachine serializes doTransition() and notifyMessage(), so a late reply cannot interleave with the timeout or close of the request it answers, and its state, address and port are volatile; its observers, the connection listeners and the connector listener lists are copy-on-write lists, so a listener may be registered or unregistered while they are notified. Connection.waitForResponse() rolls the state machine back outside the response lock, which the receiving thread takes while holding the state machine lock; ConnectionManager looks connections up without a lock for the same reason (#96). IntegrityAlgorithm and ConfidentialityAesCbc128 synchronize the use of their Mac and Cipher, which the sending, receiving and keep-alive threads share (#89). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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: 49fd688773
ℹ️ 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".
# Conflicts: # src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java
…r atomically A state that emits an action during a transition or the processing of a received message queues it; doTransition() and notifyMessage() dispatch the queued actions to the observers once they release the lock, so no listener runs under the state machine monitor. ConnectionManager.close() disconnects the connections under the same lock as connect() and marks the manager closed; connect() then throws IllegalStateException instead of creating a connection on a closed messenger. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 837a3b3b02
ℹ️ 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".
Only an in-session message, which reaches the application listeners, is dispatched after the lock is released; a handshake reply, an error and the session key are published to the connection under the lock, so the caller sees the reply together with the state it produced and cannot time the request out in between. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0331764340
ℹ️ 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".
# Conflicts: # src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java
The receiving thread publishes a reply under the state machine lock; waitForResponse() now takes that lock when the wait expires and applies the Timeout only if no reply was published meanwhile, so a reply that arrives as the deadline passes is used instead of being rolled back. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
# Conflicts: # src/test/java/org/metricshub/ipmi/core/connection/ConnectionTest.java
1 participant
Fixes #96, fixes #89. Stacked on #148 (base branch
fix/connection-lifecycle); will be retargeted tomainonce #148 is merged.Data races (#96)
StateMachine.doTransition()andnotifyMessage()are synchronized: a transition on the caller thread (send, timeout roll-back, close) and the processing of a reply on the receiving thread no longer interleave, so a late reply cannot clobber the state the timeout just rolled back.current,initialized, the remote address and port arevolatile;observersis aCopyOnWriteArrayList.Connection.listenersand the two listener lists ofIpmiAsyncConnectorareCopyOnWriteArrayLists: they wereArrayLists iterated by the receiving and timer threads while the caller thread registered and unregistered itsMessageListenerfor every message. A listener may now unregister itself from its own callback (tests).Connection.waitForResponse()rolled the state machine back from inside the response lock, which the receiving thread takes while holding the state machine lock (the roll-back now happens after the lock is released);ConnectionManagerlooked connections up under the list lock, whichcreateConnection()holds while starting a state machine (the list is copy-on-write and looked up without a lock, a private lock keeps size-then-add atomic).MessageListenerwas already correct (its fields are only touched under its monitor).Shared Mac and Cipher (#89)
IntegrityAlgorithm.initialize()/generateAuthCode()andConfidentialityAesCbc128.initialize()/encrypt()/decrypt()are synchronized: the caller, receiving and keep-alive threads share the oneMacandCipherof the session's cipher suite. Two tests hammer each algorithm from 8 threads and check the results against the single-threaded ones (without the fix, both tests fail on every run: wrong HMACs, and AES round trips that throwNegativeArraySizeExceptionon a corrupted pad byte).#97
Closed with measurements (see the issue): the reported in-JVM scenario on the GIGABYTE BMC passes 4/4 on
mainsince #140 and on this branch; the Supermicro card that still loses 2 of 4 simultaneous sessions loses them from separate JVMs and fromipmiutilas well, so that is a BMC-side limit.Verification
mvn formatter:format verifygreen on JDK 17 (SpotBugs rejectedsynchronized (CopyOnWriteArrayList), an unsynchronized getter next to a synchronized setter and a non-volatile port: fixed as above); 96 tests. Live: the #97 runs on this branch (GIGABYTE, Supermicro) log in and read the same 62 sensors asmain.🤖 Generated with Claude Code