Skip to content

Conversation

@HTHou
Copy link
Contributor

@HTHou HTHou commented Dec 19, 2025

Description

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

@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 0% with 176 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.21%. Comparing base (9386468) to head (58bd023).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/apache/iotdb/rpc/TNonblockingSSLSocket.java 0.00% 174 Missing ⚠️
...apache/iotdb/rpc/TNonblockingTransportWrapper.java 0.00% 2 Missing ⚠️
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.
📢 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.

Copy link
Contributor

Copilot AI left a 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 TNonblockingSSLSocket class that uses Java's SSLEngine for SSL/TLS handshaking and encryption/decryption
  • Removes the complex NettyTNonblockingTransport class 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.

Copy link
Contributor

Copilot AI left a 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.

@sonarqubecloud
Copy link

Copy link
Contributor

Copilot AI left a 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.

Comment on lines +214 to +215
if (appUnwrap.hasRemaining()
|| (decodedBytes.position() > 0 && decodedBytes.flip().hasRemaining())) {
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
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)) {

Copilot uses AI. Check for mistakes.
case OK:
if (appUnwrap.position() > 0) {
appUnwrap.flip();
appUnwrap.compact();
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
appUnwrap.compact();

Copilot uses AI. Check for mistakes.
public synchronized int write(ByteBuffer buffer) throws TTransportException {
int numBytes = 0;

if (buffer.position() > 0) buffer.flip();
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
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();
}

Copilot uses AI. Check for mistakes.
Comment on lines +248 to +249
if (nTransfer > 0) {
appWrap.put(buffer);
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant