fix(geohash): publish currentGeohash writes to the threads that read it - #895
Open
Chessing234 wants to merge 1 commit into
Open
fix(geohash): publish currentGeohash writes to the threads that read it#895Chessing234 wants to merge 1 commit into
Chessing234 wants to merge 1 commit into
Conversation
GeohashRepository synchronizes every accessor that touches its shared maps - eighteen of them - but currentGeohash is a plain var with two unsynchronized accessors, and it is written from the UI thread when the user picks a channel while Nostr handler and timer threads read it. Several of those reads happen inside the @synchronized methods (refreshGeohashPeople, updateParticipant, displayNameForNostrPubkey), and holding the lock on the read side buys nothing when the write side never takes it: there is no happens-before edge, so a stale value can persist. Two ways that shows up: startGeoParticipantsTimer loops `while (repo.getCurrentGeohash() != null)` and keeps refreshing a channel the user has left, and displayNameForNostrPubkey derives the identity for the wrong geohash when deciding whether a pubkey is us. Synchronize the two accessors, matching the rest of the class. NotificationManager keeps the same state @volatile for this reason.
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.
GeohashRepositorysynchronizes every accessor that touches its shared maps — eighteen of them — butcurrentGeohashis a plainvarwith two unsynchronized accessors:It is written from the UI thread when the user picks a channel (
GeohashViewModel.selectLocationChannel,MainActivity) and read from Nostr handler and timer threads. Several of those reads are inside the@Synchronizedmethods —refreshGeohashPeople,updateParticipant,displayNameForNostrPubkey— and holding the lock on the read side buys nothing when the write side never takes it: there is no happens-before edge, so a stale value can persist indefinitely.Two places that shows up:
startGeoParticipantsTimerloopswhile (repo.getCurrentGeohash() != null), so a missed write keeps refreshing participants for a channel the user has left;displayNameForNostrPubkeyuses it to derive the identity that decides whether a pubkey is us, so a stale geohash can label someone else's message with your nickname, or drop yours.Synchronizing the two accessors matches the rest of the class.
NotificationManagerholds the same state and marks it@Volatilefor this reason, so the codebase already treats it as cross-thread state../gradlew :app:testDebugUnitTest :app:lintDebugpasses locally (rc=0). No behaviour test: a missing happens-before edge cannot be observed reliably from a unit test, and asserting it would only encode a timing accident.