Repository navigation
fix(transport): guard void closePromise in NativeTlsSession second-close fan-out - #63
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
============================================
+ Coverage 86.82% 86.84% +0.01%
- Complexity 616 619 +3
============================================
Files 60 60
Lines 16979 16986 +7
Branches 490 491 +1
============================================
+ Hits 14742 14751 +9
+ Misses 1913 1910 -3
- Partials 324 325 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will degrade performance by 10.46%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | retain-drop[1048576] |
300.8 ns | 354.9 ns | -15.26% |
| ❌ | Simulation | retain-drop[4096] |
300.8 ns | 354.9 ns | -15.26% |
| ❌ | Simulation | retain-drop[64] |
300.8 ns | 354.9 ns | -15.26% |
| ❌ | Simulation | retain-drop[65536] |
300.8 ns | 354.9 ns | -15.26% |
| ⚡ | Simulation | overwrite[64] |
520.2 ns | 466.1 ns | +11.62% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing detail/bug-fix/fix-transport-guard-void-closepromise-in-nativetls-d36b45 (6328f6c) with main (26fb123)
Footnotes
-
65 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Detail bug report: View on Detail
Change
JVM transport layer (
packages/netty) —NativeTlsSession.requestClose.close_notifywith half-closure disabled,NativeStreamChannel.tlsPeerClosed()initiates a close viaunsafe().close(voidPromise()). That void promise flows throughdeferCloseintoNativeTlsSession.requestCloseand is stored asclosePromise. If any subsequentclose(promise)arrives while that TLS close is still in flight,requestClosefans out by callingclosePromise.addListener(...). Under the pinned Netty 4.2.18,VoidChannelPromise.addListenerthrowsIllegalStateException("void future")unconditionally, corrupting the second caller's promise with a bogus failure — or hanging it entirely if the close was queued from a non-event-loop thread.isVoid()guard thatNativeTlsSession.close()already applies on the sameclosePromisefield.closePromiseis void, bind the second promise tochannel.closeFuture()(which always completes once the TLS close exchange or the 5-second close-timeout fallback force-closes the channel) instead of callingaddListeneron the void promise. This mirrors Netty's own pattern for late close requests and is consistent with the existingisVoid()guard inclose().Closes #56
Validation
NativeTlsDoubleCloseTest: sets up a loopback TLS client/server on the native transport, strands the close-record write (widening the in-flight window deterministically), then issuesclose(voidPromise())followed byclose(realPromise)from the event loop. Asserts the second promise completes successfully andcloseFuturesucceeds. Verified the test fails without the fix (reproducing the exactIllegalStateException: void futurestack from the bug report) and passes with the fix.NativeTlsClosePromiseTest,NativeReentrantCloseTest,NativeTlsOrderingTest,NativeTlsChannelTest,NativeTlsNegativeTest,NativeLifecycleTest,NativeSslEngineTest, and the fullmake test-jvmcontract suite — all pass with no regressions. (NativeSslInteropTestfailed due to/usr/bin/opensslnot being executable in this environment; unrelated to this change.)make build-jvm(javac--release 22 -Xlint:all -Werror),google-java-format --dry-run --set-exit-if-changed, andtools/check_java.py(Error Prone + NullAway + Checker Framework) all pass clean.Automatic Fixes PRs can be configured here.