Skip to content

minecraft/conn.go: Re-check deferred packets on expect() to prevent handshake deadlock#406

Open
iteplenky wants to merge 1 commit intoSandertv:masterfrom
iteplenky:fix/deferred-packet-race
Open

minecraft/conn.go: Re-check deferred packets on expect() to prevent handshake deadlock#406
iteplenky wants to merge 1 commit intoSandertv:masterfrom
iteplenky:fix/deferred-packet-race

Conversation

@iteplenky
Copy link
Copy Markdown

When a server sends ResourcePacksInfo before PlayStatus(LoginSuccess),
the packet gets deferred because its ID isn't in expectedIDs yet. When
PlayStatus later arrives and expect(IDResourcePacksInfo) is called,
the already-deferred packet is never re-checked — causing Dial() to
deadlock waiting for a packet that's already been received and discarded.

This adds handleDeferredPackets() which is called from expect() to
re-process deferred packets against the updated expected set. Packets
that still don't match are re-deferred by handle() automatically.

Observed on: Hive, Lifeboat, Galaxite (intermittent, depends on packet
arrival order which varies per connection).

Comment thread minecraft/conn.go Outdated
return
}
deferred := conn.deferredPackets
conn.deferredPackets = nil
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would change this to be conn.deferredPackets = conn.deferredPackets[len(deferred):] just to keep the pre-allocated size

@HashimTheArab
Copy link
Copy Markdown
Contributor

you can just add resourcepacks info to this list

conn.expect(packet.IDServerToClientHandshake, packet.IDPlayStatus)

@TwistedAsylumMC
Copy link
Copy Markdown
Collaborator

you can just add resourcepacks info to this list

conn.expect(packet.IDServerToClientHandshake, packet.IDPlayStatus)

I guess it depends how far the client lets you go with the ordering of these packets. The PR's implementation is the most future-proof way, but if the fix you've provided is solid enough then maybe it's better to merge that over this

@HashimTheArab
Copy link
Copy Markdown
Contributor

been using it for over a year

@TwistedAsylumMC
Copy link
Copy Markdown
Collaborator

My point isn't for what works currently, it's what is best going forward so we don't need to make yet another change when a server sends a packet in a slightly different order that isn't caught by the current code

@HashimTheArab
Copy link
Copy Markdown
Contributor

ik i agree

If a packet arrives before expect() adds its ID to the expected set,
it gets deferred and never re-checked — causing the handshake to hang
indefinitely.

Fix: after storing new expected IDs, pass all deferred packets back
through handle(). Matching packets are processed immediately; the
rest are re-deferred automatically.
@iteplenky iteplenky force-pushed the fix/deferred-packet-race branch from 1ae7b9c to 195bfab Compare March 17, 2026 11:41
@iteplenky
Copy link
Copy Markdown
Author

@HashimTheArab Re: adding IDResourcePacksInfo to the initial expect() at dial.go:298 - that works for this specific case, but the root cause is that expect() never re-checks already-deferred packets. If any other packet arrives out of order in the future (e.g. ResourcePackStack before ResourcePackDataInfo completes), it would deadlock again and need another hardcoded addition.

The handleDeferredPackets() approach fixes it generically: whenever expect() updates the expected set, all deferred packets are re-evaluated. No future changes needed regardless of server packet ordering quirks.

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.

3 participants