The PlayerSpawnFinder class recursively searches for candidates, which can result in StackOverflowException if the target world does not contain valid regions around the spawn area in close proximity and the searchable chunks are loaded.
Analysis of the bug:
The PlayerSpawnFinder#scheduleNext calls PlayerSpawnFinder#scheduleCandidate which ends up requesting the chunk with call to ServerChunkCache#addTicketAndLoadWithRadius and returns a future to the callee. If the chunk is loaded, the future is already marked as complete and the following callback given in CompletableFuture#whenCompleteAsync is immediately executed in-line. This callback ends up calling the PlayerSpawnFinder#scheduleNext again and we end up in a recursive call, potentially repeating this hundreds of times.
Suggested fix:
The PlayerSpawnFinder should not perform the search using recursion and instead use iteration and only upon finding an unloaded chunk, should it schedule a continuation.
Reproduction
I have attached a world save file, which upon loading will end up being stuck in the “Loading terrain” screen forever. This world is an ocean super flat world and contains force loaded area of 16 chunks around the spawn with gamerule spawnRadius set to 16.
Linked issues
Attachments
Comments 5
Default thread stack size can differ across multiple platforms and JVM implementations, so not entirely unsurprising, but a bit unexpected given the very deep stack in the repro.
I personally tested this on Windows 24H2 coupled with the JVM that's shipped together with the Minecraft’s launcher.
You also have to replace the files if you somehow loaded in and want to try again, as that calculation is done only before the player has spawned in the world for the first time.
Regardless, the code here is flawed as it permits deep recursion (generally a bad thing and should be avoided, as the stack size is limited) and can be easily eliminated by changing the approach to be iterative.
Hi! Are you still able to reproduce this in the latest version?
This seems to be a duplicate of MC-302549 which has been resolved, though I see you were able to reproduce this on 1.21.11-pre-1 which would have already included that fix.
Looks like this is indeed fixed. The fix was implemented inside BlockableEventLoop#execute to prevent recursion from occurring in any scheduled task. Previously it would always execute the task inline when called from main thread but now it schedules it for the next tick if the task would result in recursion.
I can’t reproduce this; the world loads for me