Skip to content

test: patch Netty's lost-read-interest race in the test server via a javaagent - #4019

Closed
u9g wants to merge 1 commit into
masterfrom
test/netty-autoread-agent
Closed

test: patch Netty's lost-read-interest race in the test server via a javaagent#4019
u9g wants to merge 1 commit into
masterfrom
test/netty-autoread-agent

Conversation

@u9g

@u9g u9g commented Aug 27, 2026

Copy link
Copy Markdown
Member

Companion to #4018. That PR detects the login stall and reconnects; this one removes it on the test server.

The bug (vanilla ≤1.20.1, Paper included): Connection.sendPacket calls setAutoRead(false) on the main thread for each PLAY packet sent while the channel attr still says LOGIN, and queues setAutoRead(true) on the netty IO thread. Netty re-arms OP_READ only on a 0→1 flip of the flag, but the queued clearReadPending0 drops it unconditionally — so when doSendPacket_i runs between the main thread's toggles for i+1 and i+2, the channel ends up autoRead=true with no read interest, nothing re-arms it, and ReadTimeoutHandler(30) kills the connection. Reproduced deterministically on stock Netty 4.1.82 with latches, and on a real 1.19.3 server at ~1% of logins under CPU load (~10% with Connection DEBUG logging). Mojang rewrote protocol switching in 1.20.2 (MC-265209); Paper's archived ≤1.20.1 branches have no fix, and there is no property to toggle.

The fix: a -javaagent (test/netty-agent/NettyAutoReadFixAgent.java, source only) that rewrites AbstractNioChannel.clearReadPending0 with the JDK ClassFile API to skip removeReadOp() when autoRead is already true. Bytecode-level, so it works on whatever Netty 4.1.x the server bundles (4.1.9–4.1.82 across 1.12–1.20.1) and is a logged no-op on 1.8.8's 4.0.23. test/common/nettyAgent.js compiles and jars it at test time and the harness attaches it via JAVA_TOOL_OPTIONS, so minecraft-wrap is untouched. If no JDK is present it logs and the suite runs unpatched.

CI change: java-package: jrejdk so javac/jar exist on the runner (still Java 25).

Validation

  • 1.19.3 (Netty 4.1.82, bundler jar): patched, 70/70
  • 1.16.5 (Netty 4.1.25, shaded jar): patched, 69/69
  • 1.8.8 (Netty 4.0.23): no clearReadPending0, 69/69
  • Loaded-server login loop with DEBUG logging: 8 stalls / 60 logins unpatched → 0 / 800 patched.

@rom1504

rom1504 commented Aug 28, 2026

Copy link
Copy Markdown
Member

I'm not sure we should do this. That makes the server non vanilla and it removes a cause of failure that real users might see.
Should we not just kill the server and retry instead?

…javaagent

Vanilla servers up to 1.20.1 can stop reading a client's socket during
login: Connection.sendPacket calls setAutoRead(false) on the main thread
for every PLAY packet sent while the channel still says LOGIN and queues
setAutoRead(true) onto the IO thread. Netty re-arms OP_READ only on a
0->1 flip of the flag but the queued clearReadPending0 drops it
unconditionally, so when doSendPacket_i runs between the main thread's
toggles for i+1 and i+2 the channel ends with autoRead=true and no read
interest. Nothing re-arms it; ReadTimeoutHandler kills the connection
30s later and the whole version fails in CI. Mojang removed the toggle
in 1.20.2 (MC-265209); Paper's archived branches carry no fix.

The agent rewrites AbstractNioChannel.clearReadPending0 with the JDK
ClassFile API to leave OP_READ alone when autoRead is already true. It
is bytecode-level, so it applies to whatever Netty 4.1.x a server
bundles (4.1.9 through 4.1.82 across 1.12-1.20.1) and is a no-op on
1.8.8's Netty 4.0.23, which predates the method. The jar is compiled
from test/netty-agent at test time with the JDK's javac and jar and
attached through JAVA_TOOL_OPTIONS, so minecraft-wrap is untouched; CI
therefore needs a JDK rather than a JRE.

Reproduced the stall at ~1% of logins under CPU load and ~10% with
Connection DEBUG logging; with the agent, 0 stalls in 800 such logins.
@u9g
u9g force-pushed the test/netty-autoread-agent branch from 3e7be07 to 4d41973 Compare August 29, 2026 14:11
@u9g

u9g commented Aug 29, 2026

Copy link
Copy Markdown
Member Author

I'm not sure we should do this. That makes the server non vanilla and it removes a cause of failure that real users might see. Should we not just kill the server and retry instead?

You’re right, although it’s clientside that we should restart, done in #4018

@u9g u9g closed this Aug 29, 2026
@u9g
u9g deleted the test/netty-autoread-agent branch August 29, 2026 15:30
@u9g
u9g restored the test/netty-autoread-agent branch August 29, 2026 15:30
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.

2 participants