mirror of
https://github.com/devgianlu/go-librespot.git
synced 2026-04-26 05:15:49 +03:00
[GH-ISSUE #211] HttpChunkedReader does not follow the ReadAt guidelines leading to the program getting stuck #139
Labels
No labels
bug
enhancement
pull-request
spotify-side
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/go-librespot#139
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @tooxo on GitHub (Sep 16, 2025).
Original GitHub issue: https://github.com/devgianlu/go-librespot/issues/211
Reading the documentation for the
ReaderAtinterface, it states:Looking at the implementation of ReadAt in the HttpChunkedReader:
github.com/devgianlu/go-librespot@8d888e4e46/audio/chunked_reader.go (L223-L225)This case can (and does according to my debugger) return the tuple (0, nil), which is an illegal result value according to the documentation. I observed this, because this call
github.com/devgianlu/go-librespot@8d888e4e46/vorbis/decoder.go (L172)never returned, blocking the entire program as well as the dealer loop.
I'm not entirely sure what the correct return value would be here, i would assume (n, io.EOF), but I would like some advice on this.
Core Dump
@tooxo commented on GitHub (Sep 17, 2025):
Also tangentially related:
github.com/devgianlu/go-librespot@8d888e4e46/audio/chunked_reader.go (L140-L145)This should be
chunk.err == nil, because if the chunk load results in an error, the chunk.fetching loop busy-loops, never unlocking the lock (.Wait()un-locks the lock), deadlocking the goroutine trying to set chunk.fetching to false.E.g.:
github.com/devgianlu/go-librespot@8d888e4e46/audio/chunked_reader.go (L160-L164)I am also not sure why the condition (rather the nested loop) is there in the first place, the loop should not have the opportunity to busy-loop in any scenario, because of deadlock and performance issues.
Core Dump (This dump is quite big, interesting goroutines would be 536716, 536743 as the ones being stuck and 526121, 526124, 526127 as the ones busy looping [there are tons more, just look at it using goroutine-analyzer])
https://gist.github.com/tooxo/f85b564a5f83da75f8246ce7ff3849d8
@devgianlu commented on GitHub (Sep 18, 2025):
I don't see where the busy looping is, the goroutines that you mentioned are waiting to obtain a lock and not burning instructions in a loop.
github.com/devgianlu/go-librespot@8d888e4e46/audio/chunked_reader.go (L140-L145)The idea behind this loop is that if the chunk is already being fetched (
chunk.fetching) we wait untilchunk.dataorchunk.errbecome non-nil indicating that something has changed in the state of the chunk. If it's ready, we return early with the data, else we try to fetch it again.github.com/devgianlu/go-librespot@8d888e4e46/audio/chunked_reader.go (L160-L164)By setting
fetchingtofalseanderrto a non-nil value, we ensure that whenBroadcastis called any loop like the one mentioned above will exit.I haven't inspected your dump, but hopefully this provides some insight. It surely is strange that there's a goroutine waiting for 16 minutes to obtain a lock.
@tooxo commented on GitHub (Sep 18, 2025):
I'm sorry if I didn't articulate this clear enough, but the top comment and the answer are two issues, which are only related in the sense that both of them lock up the dealer loop somehow. Otherwise they are unrelated, so there is no busy loop in the upper core dump, but only in the lower one.
I am not sure if one can even see a busy loop in a core dump, but from what i saw in the debugger there is a scenario where err != nil and fetching is true.
Removing the inner loop will fix the issue, because from the programs logic, fetching = true should imply data and err to be nil, and fetching = false will imply one of them to be not-nil, so waiting until fetching is false would be enough anyway. Nesting these for loops only allows the described deadlock to happen. I hope the issue is more clear now.
@devgianlu commented on GitHub (Sep 19, 2025):
I see what you mean now, but I think we should focus on fixing where
err != nil and fetching is truehappens, that's the real bug. The fact that the two nested loops cause a busy loop is the effect of the bug.Perhaps we should clear
chunk.errandchunk.datahere?@tooxo commented on GitHub (Sep 19, 2025):
Yes this would also be a solution. Clearing chunk.err would be enough, because otherwise the function would have returned at this point anyway. I will amend the PR.
@devgianlu commented on GitHub (Sep 19, 2025):
This can be closed right?
@tooxo commented on GitHub (Sep 19, 2025):
yup