Isolate exceptions in processTrackQueueInParallel/flushQueueInParallel#77
Open
mliem2k wants to merge 1 commit into
Open
Isolate exceptions in processTrackQueueInParallel/flushQueueInParallel#77mliem2k wants to merge 1 commit into
mliem2k wants to merge 1 commit into
Conversation
Unlike tickRegion, which logs and marks a chunk-system crash on any exception (isolating the failure to a single region), the parallel tracked-entity and packet-flush futures had no .exceptionally() at all. Tracing the future chain up through ServerChunkCache.tick -> ServerLevel.tickAsync -> MinecraftServer's unguarded future.join() confirms an exception here isn't silently dropped: it propagates uncaught and crashes the whole tick loop, a much larger blast radius than the per-region isolation tickRegion already provides one method away. Add matching .exceptionally() handlers so a failure in one of these paths degrades the same way tickRegion's does instead of taking down the server.
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.
Summary
Found while investigating #49 (unrelated, reporting separately since it's a distinct bug).
tickRegion()isolates any exception to the single failing region: it logs and callsmoonrise\$setChunkSystemCrash(...), letting the rest of the tick continue.processTrackQueueInParallel()andflushQueueInParallel(), one method away in the same file, have no.exceptionally()at all.Traced the future chain up through
ServerChunkCache.tick()->ServerLevel.tickAsync()->MinecraftServer's per-levelfuture.join()(unguarded by any try/catch at that point). An exception thrown here, e.g. from a plugin's buggy tracked-entity hook, or from flushing a broken connection, isn't silently dropped: it propagates uncaught throughjoin()and crashes the entire tick loop, a much larger blast radius than the per-region isolationtickRegion()already provides.This adds matching
.exceptionally()handlers to both methods so a failure here degrades the same waytickRegion()'s does instead of taking the whole server down.Test plan
./gradlew :shreddedpaper-server:compileJava)