feat: add TcpStackAwareSocketChannel for z/OS TCP/IP stack recovery — GH#4776 - #4792
feat: add TcpStackAwareSocketChannel for z/OS TCP/IP stack recovery — GH#4776#4792balhar-jakub wants to merge 11 commits into
Conversation
QA + Security Review — PR #4792 (#4776)Verdict: APPROVED Build and Tests
Pavel's Lens — All 8 Rules Checked
Minor Notes (non-blocking)
Security Assessment
APPROVED — ready for merge. |
|
DCO check failed — all 3 commits are missing Fix: |
75421a5 to
44fcad4
Compare
|
balhar-jakub
left a comment
There was a problem hiding this comment.
Multi-Reviewer Review
Verdict: Approve with minor NITs.
Findings:
- ℹ️ NIT — Typo in
IMPL_CLOSE_SELECTABGLE_CHANNEL_HANLE(should beHANDLE) — pre-existing, but inherited. - ℹ️ NIT — Method counts in
ExcludedSocketOpscomment are slightly off (14/6 split is approximate). - ℹ️ NIT —
close()is excluded but not overridden — defense-in-depth viaimplCloseSelectableChannel()works. - ℹ️ NIT — No
toString()override. - ℹ️ NIT —
safeCloseswallows IOException silently; consider trace-level log. - ✅ Test coverage is excellent (every read/write variant tested, mockStatic for
NetworkRecycledException).
Good:
- Feature flag
apiml.tcpStackAwareSocketChannel.enableddefaults tofalse— safe opt-in. @Delegateapproach correctly handles 20 excluded methods + delegates the rest.isTcpStackRestartednowstatic(was instance) — cleaner since no instance state needed.- New tests cover both the existing
isTcpStackRestartedand the newTcpStackAwareSocketChannel.
Optional follow-up:
- Update
apiml-tomcat-commonCHANGELOG / docs to mention the new flag. - Consider a tenant-level integration test that toggles the flag and verifies behavior.
Required before merge: none. All findings are NIT-level polish.
…StackAwareSocketChannel config (#4776) Move isRecycledClass and isTcpStackRestarted from FixedServerSocketChannel instance methods to package-private static methods on TomcatAcceptFixConfig. Add apiml.tcpStackAwareSocketChannel.enabled config property (default true). Update call sites in FixedServerSocketChannel.accept() to use static methods. Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
…#4776) Create ExcludedSocketOps interface (20 methods) and TcpStackAwareSocketChannel inner class extending SocketChannel with @DeleGate. Intercepts read/write ops to detect EDC5122I/NetworkRecycledException, closes dead socket, re-throws. Wire into FixedServerSocketChannel.accept() guarded by config property apiml.tcpStackAwareSocketChannel.enabled (default true). Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Add comprehensive tests for TcpStackAwareSocketChannel wrapper: - Normal read/write passthrough - EDC5122I detection on read/write/scatter/gather (close + rethrow) - NetworkRecycledException detection via MockedStatic - Non-EDC5122I IOException passthrough (no close) - safeClose robustness (IOException on close ignored) - configureBlocking/isBlocking delegation - Integration test with real SocketChannel - Config disabled returns raw SocketChannel Update TcpStackRestartHandling tests to use static methods. All 76 tests pass. Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Change default from true to false. The wrapper breaks Tomcat socket acceptance on non-z/OS platforms (Linux CI runners), causing tests to hang with Connection refused until BuildAndTest times out at 35 minutes. The feature only has value on z/OS where TCP/IP stack restarts can occur — z/OS deployments must explicitly set apiml.tcpStackAwareSocketChannel.enabled=true. Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
2396fd8 to
0dfba61
Compare
|
The start.sh scripts are missing support for the new configuration property. |
|
When the feature is enabled on z/OS, APIML is not able to accept connections and logs: |
Review notes: Background: the two z/OS TCP/IP stack-restart failure modes
Review findings (PR 4792,
|
| Throwable cause = t.getCause(); | ||
| if ((cause != null) && (cause != t)) { |
There was a problem hiding this comment.
cause == null if t == cause from the t.getClause implementation. The cause != t check is unnecesary
| running.set(false); | ||
| } | ||
|
|
||
| static boolean isRecycledClass(Throwable t) { |
There was a problem hiding this comment.
is recycled is called only once. Can be internalized.
| } catch (RuntimeException e) { | ||
| log.debug("Unable to close a stale client connection after TCP/IP stack restart", e); | ||
| } | ||
| } | ||
| log.info("Closed {} client connection(s) after TCP/IP stack restart", closed); |
There was a problem hiding this comment.
It is useful to know if there are sockets that cannot be freed and why. Debug is rarely turned on in production. Suggestion to add a number of connections that could not be closed. If there is a repeating problem, the user can be asked to turn on debug and replicate.
| return NETWORK_RECYCLED_EXCEPTION_CLASS.equals(t.getClass().getName()); | ||
| } | ||
|
|
||
| static boolean isTcpStackRestarted(Throwable t) { |
There was a problem hiding this comment.
Why was the method moved from FixedServerSocketChannel class? Seems unnecessary as the method is called only from there.
| return NETWORK_RECYCLED_EXCEPTION_CLASS.equals(t.getClass().getName()); | ||
| } | ||
|
|
||
| boolean isTcpStackRestarted(Throwable t) { |
| } | ||
|
|
||
| private void closeConnectionsAfterTcpStackRestart() { | ||
| SocketWrapperBase<?>[] connections = abstractEndpoint.getConnections().toArray(SocketWrapperBase<?>[]::new); |
There was a problem hiding this comment.
getConnections returns a set. Why conversion to Array is needed?
| }).when(serverSocket).accept(); | ||
|
|
||
| assertSame(socketChannel, testEndpoint.serverSocketAccept()); | ||
| verify(staleConnection).close(); |
There was a problem hiding this comment.
It is a practice we follow to specify times(x) even for 1
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
|





Closes #4776
Problem
The z/OS TCP/IP stack can restart (EDC5122I /
NetworkRecycledException) while Tomcat is reading from or writing to an accepted client socket. Tomcat must discard the dead connection safely.Fix
isTcpStackRestartedandisRecycledClassfrom instance methods to package-private static methods onTomcatAcceptFixConfigapiml.tcpStackAwareSocketChannel.enabledconfiguration property, defaulting tofalseso non-z/OS deployments are unaffectedExcludedSocketOpsinterface for Lombok@Delegateexclusion listTcpStackAwareSocketChannelinner class:read(ByteBuffer),read(ByteBuffer[], int, int),write(ByteBuffer),write(ByteBuffer[], int, int)NetworkRecycledException, safely closes the dead socket, and re-throws so Tomcat discards the connectionConfiguration
This is a z/OS-only mitigation and is disabled by default. Enable it only for affected z/OS deployments after staging validation:
Validation
./gradlew :apiml-tomcat-common:clean :apiml-tomcat-common:build— BUILD SUCCESSFUL (77 tests)./gradlew clean && ./gradlew build— BUILD SUCCESSFUL