Skip to content

Run sync listeners of bundled packets on the main thread - #3674

Open
rishabhvenu wants to merge 1 commit into
dmulloy2:masterfrom
rishabhvenu:fix/bundle-main-thread-listeners
Open

rishabhvenu wants to merge 1 commit into
dmulloy2:masterfrom
rishabhvenu:fix/bundle-main-thread-listeners

Conversation

@rishabhvenu

Copy link
Copy Markdown

Fixes #3665

Cause

NettyChannelInjector.processOutbound() decides whether a packet has to be moved to the main thread by asking hasMainThreadListener for the packet's own type. For a bundle that is BUNDLE, but OutboundPacketListenerSet then unpacks the bundle and invokes the listeners of every packet inside it on the same thread. A synchronous listener registered only for one of the bundled packet types was therefore called on whatever thread the bundle was being sent from.

Fix

When the outbound packet is a bundle and nothing is registered for BUNDLE itself, the check now also looks at the types of the packets inside it. If any of them has a main-thread listener, the whole bundle is rescheduled to the main thread, the same way a single packet is.

The lookup only runs for bundles sent off the main thread (the thread check comes first now), and it reads the raw packet list, one registry lookup per packet, without creating containers for them. PacketContainer for the outer packet is created a few lines earlier than before so the check can use it.

Testing

  • New NettyChannelInjectorTest covers the check: a bundle containing a listened type, a bundle with a BUNDLE listener, a bundle with no listened types, and a plain packet. The first case fails if the bundle lookup is taken out again.
  • ./gradlew test passes (193 tests in paper, 16 skipped, 2 in spigot).
  • In-game on Paper 1.21.4 (build 232) with a small test plugin: a default onPacketSending listener for SYSTEM_CHAT, and a bundle of two system chat packets sent from an async scheduler thread while one client is connected.
ProtocolLib Listener thread for the two bundled packets
master (d89a5de) Craft Scheduler Thread - 3, Bukkit.isPrimaryThread() false
this branch Server thread, Bukkit.isPrimaryThread() true

In both runs a single unbundled packet sent the same way reached the listener on the server thread, and the client received all three messages.

On Paper 1.21.4 a bundle sent with sendServerPacket from an async thread was already processed on the server thread on master, so that route does not show the problem there. The test plugin therefore calls the server's Connection#sendPacket from the async thread by reflection, which is the step vanilla's Connection#send takes for a connected player.

Not covered

  • I did not test on 26.x, where the issue was reported, or on Spigot.
  • PacketFilterManager.runMonitorListeners (monitor listeners for packets sent with filters = false) makes the same per-type decision for bundles. I left it alone to keep this to the reported path.

processOutbound only checked the BUNDLE packet type for main-thread listeners before the bundle was unpacked, so a synchronous listener registered for a packet inside the bundle ran on the thread the bundle was sent from.

Fixes dmulloy2#3665

This branch has not been deployed

No deployments
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.

Outbound sync listeners may be called asynchronously for bundles

1 participant