Skip to content

Use ctx.fireChannelRead and reorder packet handler #157

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.

Sign up for GitHub

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

Closed
sanjomo wants to merge 1 commit into from
Closed

Use ctx.fireChannelRead and reorder packet handler #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,14 @@ protected Object newContinueResponse(HttpMessage start, int maxContentLength,
pipeline.addLast(HTTP_COMPRESSION, new HttpContentCompressor());
}

pipeline.addLast(PACKET_HANDLER, packetHandler);

pipeline.addLast(AUTHORIZE_HANDLER, authorizeHandler);
pipeline.addLast(XHR_POLLING_TRANSPORT, xhrPollingTransport);
if (configuration.isWebsocketCompression()) {
pipeline.addLast(WEB_SOCKET_TRANSPORT_COMPRESSION, new WebSocketServerCompressionHandler());
}
pipeline.addLast(WEB_SOCKET_TRANSPORT, webSocketTransport);

pipeline.addLast(PACKET_HANDLER, packetHandler);

pipeline.addLast(SOCKETIO_ENCODER, encoderHandler);

Expand Down
2 changes: 1 addition & 1 deletion netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ private void onPost(UUID sessionId, ChannelHandlerContext ctx, String origin, By
content = decoder.preprocessJson(jsonIndex, content);
}

ctx.pipeline().fireChannelRead(new PacketsMessage(client, content, Transport.POLLING));
ctx.fireChannelRead(new PacketsMessage(client, content, Transport.POLLING));
}

protected void onGet(UUID sessionId, ChannelHandlerContext ctx, String origin) {
Expand Down
2 changes: 1 addition & 1 deletion netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
// 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);
ctx.fireChannelRead(packetsMessage);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify where PING_TIMEOUT is cancelled and whether that logic is present
# in handlers that still receive PacketsMessage after ctx.fireChannelRead(...)

echo "== PING_TIMEOUT references =="
rg -n --type=java -C3 'PING_TIMEOUT|SchedulerKey\.Type\.PING_TIMEOUT|scheduler\.cancel\('

echo
echo "== AuthorizeHandler channelRead implementation =="
rg -n --type=java -C6 'class AuthorizeHandler|void channelRead\s*\('

echo
echo "== InPacketHandler read path =="
rg -n --type=java -C6 'class InPacketHandler|channelRead0\s*\(|channelRead\s*\('

Repository: socketio4j/netty-socketio

Length of output: 50381

🏁 Script executed:

# Get full InPacketHandler implementation to check PING_TIMEOUT handling
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/InPacketHandler.java

Repository: socketio4j/netty-socketio

Length of output: 11667

🏁 Script executed:

# Check PacketListener for any timeout-related logic
rg -n --type=java -A10 'class PacketListener|schedulePingTimeout\(\)|cancelPingTimeout\(\)' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/PacketListener.java

Repository: socketio4j/netty-socketio

Length of output: 1938

🏁 Script executed:

# Check WebSocketTransport full implementation to understand the context
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java | head -150

Repository: socketio4j/netty-socketio

Length of output: 7492

🏁 Script executed:

# Verify handler pipeline configuration
rg -n --type=java -B3 -A3 'addLast.*InPacketHandler|addLast.*WebSocketTransport|pipeline.*InPacketHandler|pipeline.*WebSocketTransport' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/

Repository: socketio4j/netty-socketio

Length of output: 51

🏁 Script executed:

# Find ChannelInitializer or pipeline configuration
rg -n --type=java 'pipeline\.addLast|ChannelInitializer' netty-socketio-core/src/main/java/com/socketio4j/socketio/ | head -50

Repository: socketio4j/netty-socketio

Length of output: 3373

🏁 Script executed:

# Check ClientHead for ping timeout scheduling details
rg -n --type=java -B2 -A8 'public void schedulePingTimeout' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/ClientHead.java

Repository: socketio4j/netty-socketio

Length of output: 519

🏁 Script executed:

# Verify the exact SchedulerKey usage in both AuthorizeHandler and ClientHead
rg -n --type=java 'new SchedulerKey.*PING_TIMEOUT' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/

Repository: socketio4j/netty-socketio

Length of output: 903

🏁 Script executed:

# Find where AuthorizeHandler.connect() is called
rg -n --type=java 'authorizeHandler\.connect|\.connect\(' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/ | head -20

Repository: socketio4j/netty-socketio

Length of output: 51

🏁 Script executed:

# Check WebSocket upgrade/handshake path
rg -n --type=java -B5 -A5 'handshake|WebSocketServerHandshaker' netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java | head -80

Repository: socketio4j/netty-socketio

Length of output: 3624

🏁 Script executed:

# Understand PacketListener.onPacket() to see when schedulePingTimeout is called
sed -n '35,150p' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/PacketListener.java

Repository: socketio4j/netty-socketio

Length of output: 3517

🏁 Script executed:

# Get connectClient implementation
sed -n '207,240p' netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java

Repository: socketio4j/netty-socketio

Length of output: 1386

🏁 Script executed:

# Check ClientHead.getBaseClient() to understand the client relationship
rg -n --type=java 'getBaseClient|class ClientHead' netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/ClientHead.java | head -5

Repository: socketio4j/netty-socketio

Length of output: 97

🏁 Script executed:

# Verify SchedulerKey equality to confirm key mismatch matters
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/scheduler/SchedulerKey.java

Repository: socketio4j/netty-socketio

Length of output: 2470

Fix the PING_TIMEOUT scheduler key mismatch between initial scheduling and cancellation.

The PING_TIMEOUT is scheduled in AuthorizeHandler.channelActive() using ctx.channel() as the key, but cancelled in AuthorizeHandler.connect() using sessionId as the key. Since SchedulerKey.equals() requires both type AND sessionId to match, these keys do not refer to the same scheduled task. When connectClient() calls authorizeHandler.connect() during the WebSocket upgrade handshake (line 216 in WebSocketTransport.java), the cancellation silently fails, leaving an orphaned timeout scheduled with the channel object. When packets later arrive and PacketListener.onPacket() reschedules the timeout using sessionId, both timeouts remain active—the original channel-keyed timeout will eventually fire and trigger a premature disconnect.

Fix by consistently using sessionId as the scheduler key for PING_TIMEOUT in AuthorizeHandler.channelActive() and AuthorizeHandler.channelRead(), or ensure connect() uses the same ctx.channel() key when cancelling.

🤖 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/WebSocketTransport.java`
at line 100, The PING_TIMEOUT scheduler key is inconsistent:
AuthorizeHandler.channelActive() schedules with ctx.channel() while
AuthorizeHandler.connect()/connectClient() cancels with a SchedulerKey built
from sessionId, so cancellation fails and leaves orphaned timeouts. Fix by
making the scheduling and cancellation use the same key: update
AuthorizeHandler.channelActive() (and channelRead() if it schedules
PING_TIMEOUT) to build the SchedulerKey using the sessionId (the same sessionId
used in connect()/connectClient()/PacketListener.onPacket()), or alternatively
change AuthorizeHandler.connect() to cancel using ctx.channel()—ensure
SchedulerKey.equals() matches by using the exact same type and sessionId symbol
(PING_TIMEOUT and sessionId) across AuthorizeHandler.channelActive(),
AuthorizeHandler.channelRead(), AuthorizeHandler.connect(), connectClient(), and
PacketListener.onPacket().

} finally {
frame.release();
}
Expand Down
Loading