Repository navigation
net: fix logins lost when FML handshakes finish at the same time - #214
Merged
Merged
Conversation
Every connection shares the one FML embedded channel. Logins finishing at once crossed its dispatcher attribute and drained each other's packets, leaving players stuck after the handshake.
A completion that reaches the server without a player's play handler used to fall through to the client-only lookup, which throws there.
Async chat broadcasts over playerEntityList on the sender's Netty thread while logins add to it, and the sender was kicked with a ConcurrentModificationException. The list stays an ArrayList because mods cast it to one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When several modded clients log in at the same moment, some of them finish the FML handshake on both sides (the client receives
HandshakeAck {3}, which the server only sends when it reachesCOMPLETE), but the server never logslogged in with entity id. The player sits in limbo until the timeout. With 10 clients connecting at once, 2 to 7 were lost in every round; with 30 at once, 8 to 11.All connections share one server-side FML
EmbeddedChannel(FMLNetworkHandler.channelPair). A handshake completes in two steps, on two different threads:forwardHandshakestores the connection's dispatcher in the shared channel'sFML_DISPATCHERattribute, writes theCompleteHandshake, and drains the shared outbound queue into the connection.HandshakeCompletionHandlerreads the dispatcher back from the shared attribute.Nothing was locked, and the attribute was read long after it was written. Two logins finishing together could:
null. The Cauldron null check then drops that completion silently.CompleteHandshakealong with its own, and the same packet object can be processed twice. That shows up asFMLMessage$OpenGui.fromBytesIndexOutOfBoundsException: the packet is[0, 1](discriminator,Side.SERVER), so the second read takes the1as theOpenGuidiscriminator, with an empty body.A second, separate race hit the same bursts. Async chat broadcasts over
ServerConfigurationManager.playerEntityList, a plainArrayList, on the sender's Netty thread, while the main thread adds the players who are logging in. The sender was kicked withInternal Exception: java.util.ConcurrentModificationException:Change
FMLNetworkHandler.forwardHandshakedoes "set attribute, write, drain, clear" insidesynchronizedon the shared channel, and removes the attribute in afinally. If the attribute were left set,FMLOutboundHandlerwould take every later FML message (OpenGui, entity spawn) for a handshake-internal one and divert it to the queue instead of the player.openGui,makeEntitySpawnAdjustment,getEntitySpawningPacketare the other writers of that channel (main thread). They take the same lock, so none of them can interleave with a handshake drain.HandshakeCompletionHandlerno longer reads the shared attribute. It resolves the dispatcher from the connection the packet arrived on:FMLProxyPacket.processPacketsetsNetworkRegistry.NET_HANDLERright before thewriteInbound, which givesNetHandlerPlayServer→netManager→NetworkDispatcher.get. The client side, with a single connection, usesgetClientToServerNetworkManager(). On the server, a completion that arrives without a player's play handler logs a warning instead of falling into the client-only lookup, which throws there. Anulldispatcher now only means the connection closed while the packet waited in the queue.playerEntityListbecomes aSnapshotIteratingArrayList, a newArrayListsubclass whose iterators (iterator,listIterator,forEach,spliterator/stream) walk a copy taken when they start. Iterating never throwsConcurrentModificationException, so the async chat loops (the broadcast, the@nameping loop andLazyPlayerSet) are safe while players join or leave. So are plugins that callBukkit.getOnlinePlayers()off the main thread, since that is a live view of this list. Writes are unchanged and stay on the main thread.Iterator.removestill removes from the live list;ListIterator.set/addthrow, with a message that points toList.set/List.add.It stays an
ArrayListon purpose. ACopyOnWriteArrayList(what CraftBukkit, Magma and Mohist use) breaks mods that cast the field. WarpTheory GTNH up to 1.5.1 does(ArrayList) playerEntityList, and GTNH 2.8.x ships WarpTheory 1.5.0. A bytecode scan of 509 mod and plugin jars found that cast, and noListIteratoruse or reflective replacement of the list.Testing
Raw 1.7.10 FML clients (
minecraft-protocolansweringFML|HSwith an empty mod list), all started at the same instant in their own containers, against a server built fromstagingwith and without this change. The server runs only its loader mods.Simultaneous logins (each client chats once, 1.5 s after joining):
HandshakeAck {3}OpenGui.fromBytesIOOBEChat during a login burst (3 rounds × 30 logins, every client chatting every 400 ms; the "before" build already has the handshake fix):
ConcurrentModificationExceptionSnapshotIteratingArrayListstress check, on Java 8 and 25: one writer adding and removing while 4 readers run for-each,forEachandstream()for 3 s. About 7 million full reads, no exception and no null element. A plainArrayListunder the same load throws within the run. The check also covers the cast toArrayList, removing during a for-each, andIterator.remove.