Repository navigation
Server: read a client's title under the server lock - #168
Merged
Merged
Conversation
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.
Why
When a new connection says HELLO with the name and address of an existing player (a title that restarted), the server clears the old connection's title (
commID) under the server lock and closes it. The old connection's own goroutine checkscommIDat the start of every request without taking that lock. If a request arrives in that window, the two accesses are unsynchronized, andgo test -racereports a data race.Change
server/server.go:handlereadscommIDunders.mu. Nothing else changes; the takeover and its replies are as before.server/commid_race_test.go: 400 rounds of a takeover while the old connection keeps sending a request. It is meant to run under-race, as CI does.Tests
go test -race ./serverpasses, and the new test passed 3 of 3 runs.server.gochange the new test fails under-racein 5 of 5 runs (WARNING: DATA RACE, write inhello, read inhandle).gofmt,go vet,python3 tools/features.py,python3 tools/site.py,python3 tools/repo_check.pyandscripts/check-deploy.shpass.Evidence
Before the change the race detector reported the write at
hello(o.commID = "") against the read at the top ofhandle, in about one of 400 takeover rounds.Not in this change
The name-takeover rules are unchanged.
Overlap
None open on
server/server.goexcept #164 (the per-address key, a different function). Touches CHANGELOG.md,site/index.htmlandsite/data.json, which other open PRs also change.Developed with AI agent assistance.