yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

Data race: the position readers guard the decoder with the speaker lock, which the read-ahead goroutine does not take #127

New Issue
Closed
opened 2026-08-19 12:47:45 +00:00 by yonlu · 1 comment
yonlu commented 2026-08-19 12:47:45 +00:00
Owner
Copy Link
Copy Source

Findings

Found by the race detector while adding a test for #123 that loads a file and lets the position be emitted while the read-ahead goroutine is running. The existing suite never does this — TestPlayer is behind YELLOWJACKET_INTEGRATION — so make test is green and the race is real:

WARNING: DATA RACE
Write at 0x00c0002c6030 by goroutine 12:
  github.com/gopxl/beep/v2/mp3.(*decoder).Stream()
  yellowjacket/backend/player.(*BufferedStreamer).readAhead()

Previous read at 0x00c0002c6030 by goroutine 9:
  github.com/gopxl/beep/v2/mp3.(*decoder).Position()
  yellowjacket/backend/player.(*Player).displayPositionSecsLocked()
  yellowjacket/backend/player.(*Player).emitPositionLocked()
  yellowjacket/backend/player.(*Player).loadFileLocked()

BufferedStreamer.srcMu exists precisely because the decoder is not safe for concurrent use, and seekLocked/rewindLocked take it through LockSource before touching the seeker. The position reads do not. displayPositionSecsLocked (player.go:1244), trackLengthLocked and CurrentPosition guard p.seeker.Position()/Len() with speaker.Lock() — a lock the read-ahead goroutine has no reason to hold and never takes. So the guard is against the wrong writer.

This runs on every position emit, which is once a second for the whole of playback, against a goroutine reading the same decoder continuously. The consequence in go-mp3 is a torn read of the decoder's position — a bogus seek-bar reading — and in principle anything, since it is undefined behaviour.

Why it is awkward to fix: seekLocked already holds srcMu for the duration of the seek and then calls emitPositionLocked inside that region, so simply taking LockSource in the position read deadlocks on a non-reentrant mutex. The guarded region needs extracting so the lock is released before the emit.

Direction

Extract the seek-and-flush part of seekLocked into a helper that owns srcMu and returns before anything is emitted, then guard the three position readers with LockSource/UnlockSource. Keep the srcMu → speaker.Lock ordering the existing callers already use.

**Findings** Found by the race detector while adding a test for #123 that loads a file and lets the position be emitted while the read-ahead goroutine is running. The existing suite never does this — `TestPlayer` is behind `YELLOWJACKET_INTEGRATION` — so `make test` is green and the race is real: ``` WARNING: DATA RACE Write at 0x00c0002c6030 by goroutine 12: github.com/gopxl/beep/v2/mp3.(*decoder).Stream() yellowjacket/backend/player.(*BufferedStreamer).readAhead() Previous read at 0x00c0002c6030 by goroutine 9: github.com/gopxl/beep/v2/mp3.(*decoder).Position() yellowjacket/backend/player.(*Player).displayPositionSecsLocked() yellowjacket/backend/player.(*Player).emitPositionLocked() yellowjacket/backend/player.(*Player).loadFileLocked() ``` `BufferedStreamer.srcMu` exists precisely because the decoder is not safe for concurrent use, and `seekLocked`/`rewindLocked` take it through `LockSource` before touching the seeker. **The position reads do not.** `displayPositionSecsLocked` (`player.go:1244`), `trackLengthLocked` and `CurrentPosition` guard `p.seeker.Position()`/`Len()` with `speaker.Lock()` — a lock the read-ahead goroutine has no reason to hold and never takes. So the guard is against the wrong writer. This runs on every position emit, which is once a second for the whole of playback, against a goroutine reading the same decoder continuously. The consequence in `go-mp3` is a torn read of the decoder's position — a bogus seek-bar reading — and in principle anything, since it is undefined behaviour. **Why it is awkward to fix**: `seekLocked` already holds `srcMu` for the duration of the seek and *then* calls `emitPositionLocked` inside that region, so simply taking `LockSource` in the position read deadlocks on a non-reentrant mutex. The guarded region needs extracting so the lock is released before the emit. **Direction** Extract the seek-and-flush part of `seekLocked` into a helper that owns `srcMu` and returns before anything is emitted, then guard the three position readers with `LockSource`/`UnlockSource`. Keep the `srcMu` → `speaker.Lock` ordering the existing callers already use.
yonlu self-assigned this 2026-08-19 12:47:51 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 12:47:52 +00:00
yonlu commented 2026-08-19 12:47:52 +00:00
Author
Owner
Copy Link
Copy Source

Same branch as #122-#126 (fix/player-playing-state) — it blocks the test added for #123, which is what found it.

Approach: extract the srcMu-owning part of seekLocked so the lock is released before emitPositionLocked runs, then guard displayPositionSecsLocked, trackLengthLocked and CurrentPosition with LockSource/UnlockSource, keeping the existing srcMu -> speaker.Lock ordering. Verified by make test, which runs the race detector.

Same branch as #122-#126 (`fix/player-playing-state`) — it blocks the test added for #123, which is what found it. Approach: extract the srcMu-owning part of `seekLocked` so the lock is released before `emitPositionLocked` runs, then guard `displayPositionSecsLocked`, `trackLengthLocked` and `CurrentPosition` with `LockSource`/`UnlockSource`, keeping the existing `srcMu` -> `speaker.Lock` ordering. Verified by `make test`, which runs the race detector.
logan closed this issue 2026-08-19 14:53:46 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-19 14:55:14 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Player Kind/Bug Platform/Android Platform/Desktop
Priority
High
2
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#127