Repository navigation
Fix the connection and manager lifecycle (#126, #94, #95, #98) - #148
bertysentry wants to merge 8 commits into
Conversation
ConnectionManager(int, long) assigns the ping period before initialize() resolves -1, so the default IpmiClient configuration gets the 30 s keep-alive of connection.properties instead of none (#126). The keep-alive task sends its Get Channel Authentication Capabilities one-way and returns: no retry loop that hot-spins when sendMessage throws, and StateMachine.stop() leaves the session state so a stopped connection reports no valid session (#94). As the keep-alive reply is no longer queued, IpmiMessageHandler delivers every reply, including the replies of that command sent by the application. The sessionless tag pool is final instead of being replaced by each new manager, every createConnection() overload returns the handle of the connection it created, closeConnection() releases the connection (getConnection() then throws IllegalStateException), getConnection( InetAddress, int) compares the addresses with equals(), and SessionManager.establishSession() closes the failed connection instead of tearing down the whole connector (#95). PropertiesManager guards its lazy initialization, skips a missing resource instead of failing with NullPointerException, keeps its map in a ConcurrentHashMap and logs lookups at DEBUG; the unused cleaningFrequency property and Constants.TIMEOUT are removed (#98). 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: ef8c6ed393
ℹ️ 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".
A one-way message skips the tags of the queued requests, so the keep-alive can never take the tag of a pending request; every operation of ConnectionManager on a released handle throws IllegalStateException; getPingPeriod() is package-private; Constants.TIMEOUT is deprecated instead of removed; the documentation of the property lookups says DEBUG and thread-safe. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8246818381
ℹ️ 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".
IpmiMessageHandler drops a reply whose command is not the command of the queued request with that tag: a late reply to the one-way keep-alive can no longer be decoded as the reply of a request that was given its sequence number since. ConnectionManager keeps its connections in a copy-on-write list looked up without a lock: the receiving thread, which holds the UDP listener lock, no longer waits for a lock that connect() and close() hold while registering or unregistering UDP listeners. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b786faa358
ℹ️ 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".
The keep-alive is a Connection.KeepAlive request, queued like any request so that its tag stays reserved until its reply arrives or it times out; IpmiMessageHandler discards the reply of a KeepAlive and delivers every other reply, including the application's own Get Channel Authentication Capabilities. The command-code check is gone. 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 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7312ecf74b
ℹ️ 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".
…eouts stay silent ConnectionManager.connect() reserves the slot under connectionsLock and registers with the messenger outside it, then disconnects and throws if close() ran meanwhile; close() flags the manager closed under the lock and disconnects outside it. A listener callback on the receiving thread, which holds the messenger lock, may therefore create or close connections without deadlocking. MessageQueue does not report the timeout of a Connection.KeepAlive, which nobody waits for; the class is public so the queue package can recognize it. The README upgrade summary lists the public changes. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd5715a133
ℹ️ 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".
IpmiMessageHandler drops a reply whose command is not the command of the queued request with that tag and leaves the request queued: a late reply to a one-way message, whose tag is not reserved, can carry the tag of a newer request. The low-level API page and the README describe the Connection.KeepAlive request. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c68ad9390
ℹ️ 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".
Command codes are scoped by network function: the reply must carry the command code of the queued request under the response network function of that request (the request one plus one, IPMI 2.0 section 5.1), and a reply whose network function the library does not know answers nothing. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c52b0ca5a2
ℹ️ 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".
The paragraph ended the Markdown table before its last rows. Co-Authored-By: Claude Fable 5.1 <[email protected]>
This branch has not been deployed
1 participant
Fixes #126, fixes #94, fixes #95, fixes #98.
Keep-alive (#126, #94)
ConnectionManager(int, long)assigned the ping period afterinitialize()had resolved-1to theconnection.propertiesvalue, so everyIpmiClientcall with the default configuration ran without keep-alive. The field is now assigned beforeinitialize(), andgetPingPeriod()exposes the result (unit test forIpmiConnector(0),(0, -1),(0, 12345)and(0, 0)).whileloop that hot-spun whensendMessagethrew (afterdisconnect(),StateMachine.stop()leftcurrentasSessionValid, so every iteration threw theState machine not startedNPE). The task now sends the message one-way and returns: no loop, no sleep, andStateMachine.stop()resets the state toUninitialized, so a disconnected connection reports no valid session.IpmiMessageHandlerno longer drops every reply whose coder class isGetChannelAuthenticationCapabilities: an application sending that command in a session gets its reply (ConnectionManager / SessionManager lifecycle bugs: static reservedTags reassigned, inconsistent handles, connections never removed, establishSession tears down the whole connector, keep-alive responses filtered by class #95.5).Manager lifecycle (#95)
reservedTagsisstatic final: it was replaced by every newConnectionManager, which discarded the reservations of the other managers of the JVM and madesynchronized (reservedTags)lock different objects over time.createConnection()overloads share one privateconnect(): the two overloads taking anint pingPeriodreturned handle 0 for every connection.closeConnection(index)releases the connection (getConnection(index)then throwsIllegalStateException; closing twice is a no-op; handles are not reused).getConnection(InetAddress, int)compares the addresses withequals().SessionManager.establishSession()closes the failed connection instead of callingtearDown()on the caller's connector (which closed its UDP socket and every other connection, e.g. theSerialOverLanmain session).PropertiesManager (#98)
Guarded lazy initialization (private lock, SpotBugs rejects
static synchronized), a missing resource is logged and skipped instead of failing withNullPointerException,ConcurrentHashMapfor the map, lookups logged atDEBUG; the never-readcleaningFrequencyproperty and the unusedConstants.TIMEOUTare removed. The file was already LF with a trailing newline in git (the CRLF of the issue is the Windows checkout).Verification
mvn formatter:format verifygreen on JDK 17 (checkstyle, PMD, CPD, SpotBugs gated); 12 new or updated unit tests, the 3 behavioral ones fail onmain.getFrusAndSensorsAsStringResulton Dell iDRAC 8 (106 s, 3 keep-alives fired), Lenovo IMM, HP iLO 4 (79 s), Supermicro and Fujitsu iRMC (99 s): identical tomainapart from reading drift. The keep-alive now actually runs during the collections that exceed 30 s.Documentation
upgrading.mdlists the user-visible changes;low-level-api.mddescribescloseConnection()as releasing the connection.🤖 Generated with Claude Code