Fix ProtocolCodeBasedDecoder memory leak #355
Conversation
WalkthroughThe changes enhance the error handling and readability of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Decoder
participant ByteBuf
Client->>Decoder: Send data
Decoder->>ByteBuf: Process data
ByteBuf->>Decoder: Check for errors
alt No error
Decoder->>Client: Return decoded data
else Error occurred
ByteBuf->>Decoder: Reset reader index
Decoder->>Client: Throw CodecException
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoder.java (1 hunks)
- src/test/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoderTest.java (3 hunks)
Additional comments not posted (18)
src/main/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoder.java (9)
82-82: Mark reader index before decoding.Marking the reader index at the beginning ensures that the buffer can be reset to this position in case of an error or incomplete packet.
83-84: Initialize protocolCode and protocol.Initializing
protocolCodeandprotocolbefore the try block ensures they are available for use within the finally block.
86-89: Handle null protocolCode.Returning early if
protocolCodeis null prevents further processing of an incomplete or invalid packet.
92-97: Set protocol and version attributes.Setting the protocol and version attributes if they are not already set ensures that the channel has the correct protocol information.
100-101: Retrieve protocol from ProtocolManager.Retrieving the protocol from
ProtocolManagerbased on the decodedprotocolCode.
102-104: Reset reader index in finally block.Resetting the reader index before throwing an exception or decoding content ensures that the buffer is in a consistent state.
107-110: Throw CodecException for unknown protocol.Throwing a
CodecExceptionif the protocol is null ensures that unknown protocols are handled appropriately.
112-112: Delegate decoding to protocol decoder.Delegating the decoding to the protocol's decoder ensures that the specific protocol's decoding logic is used.
113-116: Skip readable bytes and rethrow exception.Skipping the readable bytes and rethrowing the exception ensures that the buffer is cleared and the exception is propagated.
src/test/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoderTest.java (9)
56-56: Capture readable bytes before decoding.Capturing the number of readable bytes before decoding ensures that the buffer state can be validated after an exception.
70-71: Assert readerIndex after exception.Asserting that the
readerIndexremains consistent after an exception ensures that the buffer state is correctly maintained.
73-73: Add new test methodtestDecodeIllegalPacket2.The new test method enhances the test coverage for the decoder's behavior when handling illegal packets.
75-77: Set up EmbeddedChannel and add decoder.Setting up an
EmbeddedChanneland adding the decoder to its pipeline ensures that the decoder is tested in a realistic environment.
79-81: Write byte to ByteBuf.Writing a byte to the
ByteBufsimulates an illegal packet for the decoder to process.
82-83: Capture reader index and readable bytes.Capturing the
readerIndexandreadableBytesbefore writing to the channel ensures that the buffer state can be validated after an exception.
85-91: Write ByteBuf to channel and capture exception.Writing the
ByteBufto the channel and capturing any exceptions ensures that the decoder's error handling is tested.
92-94: Assert readerIndex after exception.Asserting that the
readerIndexremains consistent after an exception ensures that the buffer state is correctly maintained.
96-96: Assert ByteBuf reference count is zero.Asserting that the
ByteBufreference count is zero ensures that the memory leak has been resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoder.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoder.java
|
这个CI好像有问题，线下跑的单测一点问题没有，CI跑就报错 |
JoeCqupt
changed the title
我跟你有相同的问题 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoder.java (1 hunks)
Additional comments not posted (9)
src/main/java/com/alipay/remoting/codec/ProtocolCodeBasedDecoder.java (9)
82-82: Mark reader index before decoding.Marking the reader index before decoding ensures that the buffer can be reset to this position if decoding fails. This is a good practice for maintaining the integrity of the buffer state.
The code changes are approved.
83-84: Initialize protocolCode and protocol variables.Initializing these variables at the beginning of the try block ensures they are available throughout the block. This is a good practice for clarity and scope management.
The code changes are approved.
86-90: Check for null protocolCode.The check for
protocolCode == nullensures that the method returns early if the protocol code cannot be decoded. This prevents further processing of an invalid buffer.The code changes are approved.
92-97: Set protocol and version attributes.Setting the protocol and version attributes on the channel ensures that the correct protocol and version are used for subsequent processing. This is a good practice for maintaining protocol consistency.
The code changes are approved.
100-101: Retrieve protocol from ProtocolManager.Retrieving the protocol from
ProtocolManagerensures that the correct protocol is used for decoding. This is a good practice for modularity and reusability.The code changes are approved.
102-104: Reset reader index in finally block.Resetting the reader index in the finally block ensures that the buffer state is restored before any exceptions are thrown or content is decoded. This is a good practice for maintaining buffer integrity.
The code changes are approved.
107-110: Check for null protocol and throw exception.The check for
protocol == nulland throwing aCodecExceptionensures that any unknown protocol codes are handled appropriately. This is a good practice for robust error handling.The code changes are approved.
112-113: Delegate decoding to protocol decoder.Delegating the decoding to the protocol's decoder ensures that the correct decoding logic is used for the protocol. This is a good practice for modularity and reusability.
The code changes are approved.
114-116: Skip readable bytes and rethrow exception.Skipping the readable bytes and rethrowing the exception ensures that the buffer is cleared and the exception is propagated. This is a good practice for maintaining buffer integrity and proper error handling.
The code changes are approved.
|
Hi, @JoeCqupt , I doubt if it is a bug of bolt. ByteBuf will be released at |
yep. you are right. |
reproduce code
when
ProtocolCodeBasedDecoder#decode()throw exception . ByteBuf param not releasedSummary by CodeRabbit
New Features
Bug Fixes