Conversation
Replace pipeline.fireChannelRead(...) with ctx.fireChannelRead(...) in PollingTransport and WebSocketTransport so events are propagated from the current handler context (next handler) instead of from the pipeline head. Move PACKET_HANDLER registration in SocketIOChannelInitializer to after the transport handlers (but before the encoder) to ensure correct handler ordering and avoid double-processing. Changes affect SocketIOChannelInitializer.java, PollingTransport.java, and WebSocketTransport.java.
📝 WalkthroughWalkthroughThe pull request reorders the Netty pipeline handler initialization in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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
🤖 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/transport/WebSocketTransport.java`:
- 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().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07df6160-fb66-4881-b24d-93ec7ba962e2
📒 Files selected for processing (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java
| PacketsMessage packetsMessage = new PacketsMessage(client, frame.content().retain(), Transport.WEBSOCKET); | ||
| try { | ||
| ctx.pipeline().fireChannelRead(packetsMessage); | ||
| ctx.fireChannelRead(packetsMessage); |
There was a problem hiding this comment.
🧩 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.javaRepository: 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.javaRepository: 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 -150Repository: 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 -50Repository: 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.javaRepository: 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 -20Repository: 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 -80Repository: 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.javaRepository: 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.javaRepository: 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 -5Repository: 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.javaRepository: 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().
1 participant
Replace pipeline.fireChannelRead(...) with ctx.fireChannelRead(...) in PollingTransport and WebSocketTransport so events are propagated from the current handler context (next handler) instead of from the pipeline head. Move PACKET_HANDLER registration in SocketIOChannelInitializer to after the transport handlers (but before the encoder) to ensure correct handler ordering and avoid double-processing. Changes affect SocketIOChannelInitializer.java, PollingTransport.java, and WebSocketTransport.java.
Description
Brief description of the changes in this PR.
Type of Change
Related Issue
Closes #(issue number)
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit