ServerLoginPacketListenerImpl uses Executors.newCachedThreadPool with unbounded queue

Timings or Profile link

N/A

Description of issue

Discussed internally- no timings link or plugin list. This is an issue created so I can reference it in a near-future PR.

Commit 0ed4b91 reverts ThreadPoolExecutor to Executors#newCachedThreadPool which was changed to solve an issue of hanging logins. The problem with both, as discussed, is that both have the unfortunate side-effect of using one thread in the pool when combined with an unbounded queue. In the case of ServerLoginPacketListenerImpl this means that ALL logins are sent to the same thread regardless of how many threads the pool has available.

Discussions lead to a few different solutions, but after looking at the benefits and drawbacks of each solution the easiest fix is to use a fixed thread pool (a new ThreadExecutor where the core pool size is equal to the maximum pool size).

Other resolutions discussed were using a bounded queue and a "scaling thread pool" but ultimately the overhead for a fixed thread pool is minimal, is the easiest to maintain, and is likely the best solution for an authentication pool which is expected to live for the lifetime of the server anyway.

Further, discussions of a bounded vs unbounded queue led us to consider potential attack vectors. A bounded queue for the pool would see a successful attack by way of flooding the authentication service with fake logins and an unbounded pool would see a successful attack by way of flooding the authentication service with fake logins. The former would lead to RejectedExecutionExeptions and the latter would fill the heap until the server ran out of memory.

After deliberation, it was decided that a bounded queue would have less impact on the server despite a successful L7 attack on the authentication service. There are other solutions to L7 attacks on Minecraft servers and Paper's due diligence on attacks is best-effort while still keeping the original goals of performance, stability, and bugfixes in mind. Rejected logins are a better failure case than a crashed server even though the latter is more difficult.

A semi-arbitrary number of 1024 for the queue size seems reasonable.

Plugin and Datapack List

N/A

Server config files

N/A

Paper version

1.19.3 380, 0ed4b91

Other

No response

Start by locating ServerLoginPacketListenerImpl and reviewing commit 0ed4b91 to understand the executor change that caused the issue. Compare the discussed fixed-pool and bounded-queue approaches, then verify that authentication work no longer funnels through one thread and that the selected queue limit handles rejected logins without exhausting server memory.