Conversation
📝 WalkthroughWalkthroughChanges add explicit reference-count handling and safer buffer usage: Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant Transport
end
rect rgba(255,230,200,0.5)
participant PacketsMessage
end
rect rgba(255,200,200,0.5)
participant PacketDecoder
end
rect rgba(230,200,255,0.5)
participant InPacketHandler
end
Client->>Transport: WebSocket/Binary or Polling POST (ByteBuf)
Transport->>Transport: content.retain()
Transport->>PacketsMessage: new PacketsMessage(content)
Transport->>Pipeline: fireChannelRead(PacketsMessage)
PacketsMessage->>PacketDecoder: preprocessJson(content) (in-place)
PacketDecoder->>PacketsMessage: returns same ByteBuf
PacketsMessage->>InPacketHandler: delivered for handling
InPacketHandler->>InPacketHandler: checks content.refCnt() before logging
InPacketHandler->>Pipeline: continue processing / release when appropriate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes Netty IllegalReferenceCountException (issue #152) by correcting ByteBuf ownership / reference-count handling as websocket (and related) payloads are forwarded through the Netty pipeline.
Changes:
- Retain
WebSocketFramecontent before forwarding it downstream, then release the frame. - Make
PacketsMessageimplementReferenceCountedso Netty handlers can manage buffer lifetimes via auto-release. - Change
PacketDecoder.preprocessJsonto mutate the providedByteBufin-place (avoiding derived-buffer refCnt pitfalls).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java |
Retains frame content before firing PacketsMessage downstream. |
netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketDecoder.java |
Switches JSON preprocessing to in-place mutation to avoid slice/refCnt issues. |
netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java |
Implements ReferenceCounted to participate in Netty’s ref-count lifecycle management. |
netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/InPacketHandler.java |
Avoids toString() on already-released buffers in error logging. |
Comments suppressed due to low confidence (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java:35
PacketsMessagenow implementsReferenceCounted, which meansSimpleChannelInboundHandler(e.g.,InPacketHandler) will auto-release it, cascading tocontent.release(). Call sites that also release the original owner (e.g.,FullHttpRequest.release()inPollingTransport) will end up double-releasing the sameByteBufunless theyretain()before wrapping it. Please update the producers (notably the polling path) toretain()theByteBufwhen creatingPacketsMessage, or disable autoRelease / change ownership rules so each buffer is released exactly once.
public class PacketsMessage implements ReferenceCounted {
private final ClientHead client;
private final ByteBuf content;
private final Transport transport;
public PacketsMessage(ClientHead client, ByteBuf content, Transport transport) {
this.client = client;
this.content = content;
this.transport = transport;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java
Outdated
Show resolved
Hide resolved
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java (1)
25-86:⚠️ Potential issue | 🟠 MajorPolling transport double-releases reference-counted content.
PollingTransport.onPost()wrapsreq.content()directly inPacketsMessageand fires it downstream (line 172), then the finally block callsreq.release()(line 129). SincepreprocessJson()mutates in-place without creating a new buffer, the same buffer is passed toInPacketHandler, which extendsSimpleChannelInboundHandler<PacketsMessage>. Netty'sSimpleChannelInboundHandlerautomatically callsrelease()after processing, causing a double-release:
req.release()decrements refCnt (finally block, line 129)SimpleChannelInboundHandler.release()callsPacketsMessage.release()→content.release()againThe
WebSocketTransportpath correctly calls.retain()before wrapping (line 98), leaving one reference for the downstream auto-release. Both paths must follow the same pattern: either all producers retain before wrapping, or move the retain into thePacketsMessageconstructor and remove upstream retains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java` around lines 25 - 86, PacketsMessage currently wraps a ByteBuf without taking ownership, causing double-release when upstream also releases; change ownership consistently by having the PacketsMessage constructor call content.retain() (and keep the existing release/refCnt/retain/touch methods unchanged) so any producer (e.g., PollingTransport.onPost) can safely release its original req and downstream InPacketHandler / SimpleChannelInboundHandler will release the PacketsMessage-held buffer exactly once; update any code paths that previously called retain (WebSocketTransport) to remove redundant retains if present so the retain semantics are centralized in the PacketsMessage constructor and not duplicated upstream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketDecoder.java`:
- Around line 71-82: In PacketDecoder (inside the block where jsonIndex != null
and after calling replaceEscapedNewlinesInPlace), explicitly validate that the
JSONP payload is at least 2 bytes long before attempting to check the 'd='
prefix; if content.readableBytes() < 2, throw an IllegalArgumentException with a
clear message (e.g., "Invalid JSONP format: payload too short for 'd=' prefix")
so short/malformed payloads fail fast instead of bypassing the prefix check;
preserve the existing logic that checks content.getByte(ri) and
content.getByte(ri + 1) and advances content.readerIndex(ri + 2) when valid.
---
Outside diff comments:
In
`@netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java`:
- Around line 25-86: PacketsMessage currently wraps a ByteBuf without taking
ownership, causing double-release when upstream also releases; change ownership
consistently by having the PacketsMessage constructor call content.retain() (and
keep the existing release/refCnt/retain/touch methods unchanged) so any producer
(e.g., PollingTransport.onPost) can safely release its original req and
downstream InPacketHandler / SimpleChannelInboundHandler will release the
PacketsMessage-held buffer exactly once; update any code paths that previously
called retain (WebSocketTransport) to remove redundant retains if present so the
retain semantics are centralized in the PacketsMessage constructor and not
duplicated upstream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71f0cac8-3b06-423d-a320-c3ecde509d09
📒 Files selected for processing (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/InPacketHandler.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketDecoder.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java
netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketDecoder.java
Show resolved
Hide resolved
|
@NeatGuyCoding we need this in PollingTransport also in SSL right?? |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@sanjomo Seems not, it does not same to Websocket And there are some tests which you previously added always failed, can you check them please? |
There was a problem hiding this comment.
Pull request overview
This PR targets issue #152 by adjusting Netty ByteBuf reference-counting/ownership in the inbound message path to prevent IllegalReferenceCountException when using WSS/TLS and newer Netty versions.
Changes:
- Retain
WebSocketFramecontent before forwarding it asPacketsMessage, then release the frame safely. - Make
PacketsMessageimplementReferenceCountedso it can participate in Netty auto-release semantics. - Simplify
PacketDecoder.preprocessJsonto mutate the providedByteBufin-place (avoiding derived buffers) and harden error logging against released buffers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java | Retains WebSocket frame content before re-firing to the pipeline, releases frame in finally. |
| netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketDecoder.java | preprocessJson now mutates the input buffer in-place and returns it (no slice/release management). |
| netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java | Implements ReferenceCounted and delegates ref-count ops to underlying ByteBuf. |
| netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/InPacketHandler.java | Avoids calling toString() on a released buffer in the exception path. |
Comments suppressed due to low confidence (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/messages/PacketsMessage.java:33
PacketsMessagenow implementsReferenceCountedand delegatesrelease()tocontent.release(). SinceInPacketHandleris aSimpleChannelInboundHandlerwith auto-release enabled, every inboundPacketsMessagewill now release itsByteBufautomatically. However,PollingTransportcurrently createsnew PacketsMessage(client, req.content(), ...)and then unconditionallyreq.release()in itschannelReadfinally block (seePollingTransport.java:103-116+onPost(...)). That results in the sameByteBufbeing released twice (once viaPacketsMessageauto-release, once viaFullHttpRequest.release()), which can triggerIllegalReferenceCountExceptionunder load.
To fix, ensure every PacketsMessage creator transfers ownership consistently (e.g., call retain() on the ByteBuf when constructing PacketsMessage if the original parent message will be released later), and/or adjust the request/frame release strategy so the content is only released once.
public class PacketsMessage implements ReferenceCounted {
private final ClientHead client;
private final ByteBuf content;
private final Transport transport;
public PacketsMessage(ClientHead client, ByteBuf content, Transport transport) {
this.client = client;
this.content = content;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // WebSocketFrame is reference-counted and will be released below. | ||
| // Retain its content since we pass it further down the pipeline. | ||
| PacketsMessage packetsMessage = new PacketsMessage(client, frame.content().retain(), Transport.WEBSOCKET); | ||
| try { | ||
| ctx.pipeline().fireChannelRead(packetsMessage); | ||
| } finally { | ||
| frame.release(); | ||
| } |
@NeatGuyCoding and in Polling also same problem (I replicated in my local) |
|
@NeatGuyCoding full logs here
|
|
@sanjomo Sorry, I made a mistake, thanks for clarifying |
Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>
|
@sanjomo Fixed, Kindly check |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java (1)
162-175:⚠️ Potential issue | 🟡 MinorRemove the concern about
preprocessJsonreturning a different buffer; it mutates in-place. However, add exception-handling protection for the retained buffer.The
retain()call correctly addresses theIllegalReferenceCountExceptionby keeping the buffer alive afterreq.release()in the finally block. However, the original review's concern aboutpreprocessJsonreturning a differentByteBufis incorrect—the implementation mutates the buffer in-place (as documented on lines 67–68 ofPacketDecoder.java) and returns the same buffer. No leak occurs frompreprocessJson.The valid remaining concern is the exception path leak: if an exception occurs between
retain()(line 162) andfireChannelRead()(line 169)—such as frompreprocessJsonthrowingUnsupportedEncodingException—the retained buffer will not be explicitly released, creating a reference-count imbalance.Wrap the risky section in a try-catch to ensure cleanup on exception:
♻️ Suggested: Add exception handling
content = content.retain(); - // release POST response before message processing - ctx.channel().writeAndFlush(new XHRPostMessage(origin, sessionId)); - - Boolean b64 = ctx.channel().attr(EncoderHandler.B64).get(); - if (b64 != null && b64) { - Integer jsonIndex = ctx.channel().attr(EncoderHandler.JSONP_INDEX).get(); - content = decoder.preprocessJson(jsonIndex, content); - } - - ctx.pipeline().fireChannelRead(new PacketsMessage(client, content, Transport.POLLING)); + try { + // release POST response before message processing + ctx.channel().writeAndFlush(new XHRPostMessage(origin, sessionId)); + + Boolean b64 = ctx.channel().attr(EncoderHandler.B64).get(); + if (b64 != null && b64) { + Integer jsonIndex = ctx.channel().attr(EncoderHandler.JSONP_INDEX).get(); + content = decoder.preprocessJson(jsonIndex, content); + } + + ctx.pipeline().fireChannelRead(new PacketsMessage(client, content, Transport.POLLING)); + } catch (Exception e) { + content.release(); + throw e; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java` around lines 162 - 175, PollingTransport calls content.retain() then does preprocessing and fires a PacketsMessage, but if an exception (e.g. from decoder.preprocessJson) is thrown between retain() and ctx.pipeline().fireChannelRead(...) the retained ByteBuf will leak; wrap the block from after content.retain() through ctx.pipeline().fireChannelRead(...) in a try-catch (catch Throwable), and in the catch ensure any retained ByteBuf is released (release the current content; if preprocessJson may return a different ByteBuf handle both the original and returned buffers appropriately), then rethrow or propagate the exception; reference PollingTransport, decoder.preprocessJson, EncoderHandler.B64/JSONP_INDEX and PacketsMessage to locate where to add the try-catch and release calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java`:
- Around line 162-175: PollingTransport calls content.retain() then does
preprocessing and fires a PacketsMessage, but if an exception (e.g. from
decoder.preprocessJson) is thrown between retain() and
ctx.pipeline().fireChannelRead(...) the retained ByteBuf will leak; wrap the
block from after content.retain() through ctx.pipeline().fireChannelRead(...) in
a try-catch (catch Throwable), and in the catch ensure any retained ByteBuf is
released (release the current content; if preprocessJson may return a different
ByteBuf handle both the original and returned buffers appropriately), then
rethrow or propagate the exception; reference PollingTransport,
decoder.preprocessJson, EncoderHandler.B64/JSONP_INDEX and PacketsMessage to
locate where to add the try-catch and release calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adfd12b9-fc30-47d9-bf03-726cd29060fe
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java
Description
Brief description of the changes in this PR.
Type of Change
Related Issue
Closes #152
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit