[ISSUE #10395] Fix native memory leak on TLS certificate hot-reload

[qianye1001]

Summary

Fix native memory leak caused by old SslContext not being released during TLS certificate hot-reload when using the OpenSSL (netty-tcnative) provider.

Fixes #10395

Root Cause

When TLS certificates are dynamically reloaded via FileWatchService, the loadSslContext() methods in NettyRemotingServer and ProxyAndTlsProtocolNegotiator directly overwrite the sslContext field without releasing the old instance. Since ReferenceCountedOpenSslContext allocates native off-heap memory (SSL_CTX, X509 chain, EVP_PKEY), this native memory is leaked on every certificate rotation cycle.

Changes

remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingServer.java

• Capture old sslContext before building the replacement
• Call ReferenceCountUtil.release(oldSslContext) after successful assignment
• Added imports for SslContext and ReferenceCountUtil

proxy/src/main/java/org/apache/rocketmq/proxy/grpc/ProxyAndTlsProtocolNegotiator.java

• Same pattern: capture old context, release after new one is assigned
• Made sslContext field volatile for thread-safe reads from TLS handshake threads
• Added import for shaded ReferenceCountUtil

Safety

• "Build new, then release old" ordering ensures sslContext is never null or prematurely released
• ReferenceCountUtil.release() is safe for JDK provider SslContext (no-op for non-ReferenceCounted)
• In-flight connections using the old context are unaffected (refCount managed by Netty channel pipeline)

Testing

• Existing TLS tests pass
• Manual RSS monitoring during certificate rotation shows stable native memory

---

This PR was automatically generated based on the approved fix proposal.

[qianye1001]

Review by rocketmq-reviewer-bot

Summary

Fix native memory leak caused by old SslContext not being released during TLS certificate hot-reload when using the OpenSSL (netty-tcnative) provider. The fix captures the old SslContext before building the replacement and calls ReferenceCountUtil.release() after successful assignment.

Findings

• [Low] ProxyAndTlsProtocolNegotiator.java:114 — loadSslContext() is not synchronized. If called concurrently (e.g., from multiple FileWatchService callbacks), two threads could both capture the same oldSslContext reference and both call release(), causing a double-release (IllegalReferenceCountException). In practice FileWatchService uses a single callback thread, so this is low risk — but adding synchronized to the method or a lock would eliminate the race entirely.

• [Low] ProxyAndTlsProtocolNegotiator.java:81 — Good that volatile was added to sslContext. The parent class NettyRemotingAbstract already declares sslContext as protected volatile, so NettyRemotingServer is covered. ✅

• [Info] NettyRemotingServer.java:188-191 — Release ordering is correct: "build new → assign → release old" ensures no window where sslContext is null or prematurely freed.

• [Info] Both files — ReferenceCountUtil.release() is a safe no-op for JDK SslContext (non-ReferenceCounted), so this fix works correctly for both OpenSSL and JDK providers.

• [Suggestion] Missing automated test coverage. The PR checklist shows unchecked test boxes. Consider adding a unit test that:

  1. Creates an OpenSSL SslContext via loadSslContext()
  2. Calls loadSslContext() again (simulating cert reload)
  3. Asserts the old context's refCnt() is 0 after reload
     This would prevent regression and validate the fix.

• [Suggestion] PR body mentions "manual RSS monitoring" for testing — for a memory leak fix, an automated test or at least a step-by-step reproduction/verification procedure would strengthen confidence.

Cross-repo Note

No cross-repo impact. This change is internal to the broker/proxy TLS subsystem.

Verdict

Approve with minor suggestions — The fix is correct and addresses a real native memory leak. The "build new, then release old" ordering is sound. Thread safety is adequate (volatile on both field declarations). The main improvement would be adding automated test coverage.

---

Automated review by rocketmq-reviewer-bot

[qianye1001]

⚠️ Process Notice

This PR was submitted without prior community approval via /approve. Per our automated workflow, the fix proposal should have been posted as a comment on the issue first, waiting for community feedback before generating a PR.

If this fix is not desired or the approach is wrong, please close this PR.

If the fix direction is acceptable, please comment /approve on Issue #10395 and we will proceed with the review process.

Apologies for the premature submission.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.95%. Comparing base (c9e51d6) to head (7d55555).

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10396      +/-   ##
=============================================
- Coverage      49.09%   48.95%   -0.14%     
+ Complexity     13507    13472      -35     
=============================================
  Files           1376     1376              
  Lines         100537   100543       +6     
  Branches       12983    12985       +2     
=============================================
- Hits           49357    49225     -132     
- Misses         45184    45304     +120     
- Partials        5996     6014      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.