-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implement a new TNonblockingSSLSocket without using netty #16935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16935 +/- ##
============================================
+ Coverage 39.07% 39.21% +0.13%
- Complexity 207 212 +5
============================================
Files 5034 5050 +16
Lines 334594 334985 +391
Branches 42619 42685 +66
============================================
+ Hits 130750 131370 +620
+ Misses 203844 203615 -229 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR replaces the Netty-based NettyTNonblockingTransport implementation with a new native Java NIO-based TNonblockingSSLSocket implementation, eliminating the dependency on Netty libraries and removing the workaround that used fake local socket connections to trigger NIO selector events.
- Introduces a new
TNonblockingSSLSocketclass that uses Java's SSLEngine for SSL/TLS handshaking and encryption/decryption - Removes the complex
NettyTNonblockingTransportclass with its dummy channel mechanism - Removes all Netty dependencies from the service-rpc module's pom.xml
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 25 comments.
| File | Description |
|---|---|
| TNonblockingSSLSocket.java | New implementation providing SSL/TLS support using Java's SSLEngine API with buffer management for encryption/decryption operations |
| TNonblockingTransportWrapper.java | Updated to use the new TNonblockingSSLSocket instead of NettyTNonblockingTransport and changed exception handling from TTransportException to generic Exception |
| NettyTNonblockingTransport.java | Removed the entire Netty-based implementation that previously used dummy local socket channels to integrate with Thrift's selector-based async framework |
| pom.xml | Removed four Netty dependencies (netty-common, netty-buffer, netty-transport, netty-handler) from the service-rpc module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingTransportWrapper.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingTransportWrapper.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Outdated
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
| if (appUnwrap.hasRemaining() | ||
| || (decodedBytes.position() > 0 && decodedBytes.flip().hasRemaining())) { |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition on line 215 modifies decodedBytes by calling flip() as a side effect within the condition check. This is problematic because if appUnwrap.hasRemaining() is true, the flip() on decodedBytes is never executed due to short-circuit evaluation, leading to inconsistent state. Additionally, evaluating flip() inside a boolean expression is confusing and error-prone. Extract the flip() calls outside the condition.
| if (appUnwrap.hasRemaining() | |
| || (decodedBytes.position() > 0 && decodedBytes.flip().hasRemaining())) { | |
| // Determine whether decodedBytes would have remaining data after a flip, | |
| // without mutating the original buffer during condition evaluation. | |
| ByteBuffer decodedBytesView = decodedBytes.asReadOnlyBuffer(); | |
| decodedBytesView.flip(); | |
| boolean decodedHasRemaining = decodedBytesView.hasRemaining(); | |
| if (appUnwrap.hasRemaining() | |
| || (decodedBytes.position() > 0 && decodedHasRemaining)) { |
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
| case OK: | ||
| if (appUnwrap.position() > 0) { | ||
| appUnwrap.flip(); | ||
| appUnwrap.compact(); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The appUnwrap buffer manipulation logic has a potential issue. On lines 351-353, when status is OK and appUnwrap.position() > 0, the code calls flip() followed immediately by compact(). The flip() sets limit to position and position to 0, then compact() copies remaining data to the beginning. This sequence doesn't make sense - flip() is typically used to prepare for reading, but compact() is used after reading to prepare for writing. This likely leaves data in the wrong state.
| appUnwrap.compact(); |
iotdb-client/service-rpc/src/main/java/org/apache/iotdb/rpc/TNonblockingSSLSocket.java
Show resolved
Hide resolved
| public synchronized int write(ByteBuffer buffer) throws TTransportException { | ||
| int numBytes = 0; | ||
|
|
||
| if (buffer.position() > 0) buffer.flip(); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer position manipulation appears incorrect. When buffer.position() > 0, the code calls buffer.flip() which sets the limit to the position and position to 0. However, this assumes the buffer was being written to. If the buffer was already prepared for reading (position already at 0), this flip operation could cause incorrect behavior. Consider checking the buffer state or documenting the expected buffer state on entry.
| if (buffer.position() > 0) buffer.flip(); | |
| // Only flip if the buffer appears to be in write-mode (just filled, not yet flipped) | |
| if (buffer.position() > 0 && buffer.limit() == buffer.capacity()) { | |
| buffer.flip(); | |
| } |
| if (nTransfer > 0) { | ||
| appWrap.put(buffer); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to appWrap.put(buffer) on line 249 transfers nTransfer bytes from buffer to appWrap. However, ByteBuffer.put(ByteBuffer) transfers all remaining bytes from the source buffer, not a specific number. This will cause a BufferOverflowException when buffer has more bytes remaining than appWrap can hold. Use a bounded transfer by slicing the buffer or using put with offset and length.
| if (nTransfer > 0) { | |
| appWrap.put(buffer); | |
| if (nTransfer > 0) { | |
| int originalLimit = buffer.limit(); | |
| if (nTransfer < buffer.remaining()) { | |
| buffer.limit(buffer.position() + nTransfer); | |
| } | |
| appWrap.put(buffer); | |
| buffer.limit(originalLimit); |



Description
This PR implements a new TNonblockingSSLSocket without using netty. It also removed the fake connection that trigger the NIO event.