fix: data race in loginInboundConn during Forge login relay#690
Merged
Conversation
loginInboundConn's mutable state (outstandingResponses map, loginMessagesToSend deque, isLoginEventFired, onAllMessagesHandled) is accessed from two goroutines: SendLoginPluginMessage runs on event-handler / relay goroutines while handleLoginPluginResponse runs on the client read loop. The fields had no synchronization, producing a data race on the outstandingResponses map (write at SendLoginPluginMessage vs read/delete + len() in handleLoginPluginResponse), reproducible under `go test -race` via TestModernForgeIntegration_FullJoinFlow. Guard the shared fields with a mutex. External callbacks (the message consumer and onAllMessagesHandled) and connection I/O (WritePacket / BufferPacket / Flush) are invoked without the lock held to avoid re-entrant deadlocks, since a consumer may call back into SendLoginPluginMessage. Behavior is otherwise unchanged: the all-handled callback still fires after the consumer runs when no responses remain. Adds TestLoginInboundConn_ConcurrentSendAndResponse, a focused regression test that races the two methods and fails under -race before the fix.
Deploying gate-minekube with
|
| Latest commit: |
6b2a127
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://95182faa.gate-minekube.pages.dev |
| Branch Preview URL: | https://fix-login-inbound-race.gate-minekube.pages.dev |
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
1 participant
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.
Bug
loginInboundConn's mutable state is accessed from two goroutines without synchronization:SendLoginPluginMessageruns on event-handler / Forge-relay goroutines (writesoutstandingResponses, pushesloginMessagesToSend).handleLoginPluginResponseruns on the client read loop (reads/deletesoutstandingResponses, readslen()+onAllMessagesHandled).This is a real data race on the
outstandingResponsesmap (and the other fields), reproducible today undergo test -race ./pkg/edition/java/proxy/viaTestModernForgeIntegration_FullJoinFlow(writelogin_inbound.go:97↔ read:112/:116/:119/:124). It was introduced with the modern-Forge login relay (#680/#688), which drives these methods from separate goroutines.Fix
Guard the four shared fields (
outstandingResponses,loginMessagesToSend,isLoginEventFired,onAllMessagesHandled) with async.Mutex.Key detail: external callbacks and connection I/O are invoked without the lock held — the message consumer and
onAllMessagesHandledmay re-enterSendLoginPluginMessage(which takes the same lock), so holding it across those calls would deadlock. ThehandleLoginPluginResponseflow takes the lock to look up + delete the consumer, releases it to run the consumer, then re-takes it to check whether all messages are handled. This preserves the original ordering (the all-handled callback fires after the consumer runs, so a consumer that queues follow-up messages correctly defers completion).No behavior change beyond synchronization. All four guarded fields are confined to
login_inbound.go(verified), so the lock fully covers them.Test (TDD)
TestLoginInboundConn_ConcurrentSendAndResponseracesSendLoginPluginMessageandhandleLoginPluginResponseon one connection. It reports a data race under-racebefore the fix and passes after.Verification
go test -race ./pkg/edition/java/proxy/— clean (the new test, the previously-failingTestModernForgeIntegration_FullJoinFlow, and the full package)go build ./...— cleango test ./...— all packages pass (0 failures)gofmt— cleanIndependent of #689 (both branch from
master; this touches onlylogin_inbound.go).