mirror of
https://github.com/librespot-org/librespot.git
synced 2026-04-27 08:15:50 +03:00
[GH-ISSUE #437] Crash after running for some time - sequence overflow #281
Labels
No labels
A-Alsa
SpotifyAPI
Tokio 1.0
audio
bug
can't reproduce
compilation
dependencies
duplicate
enhancement
good first issue
help wanted
high priority
imported
imported
invalid
new api
pull-request
question
reverse engineering
wiki
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/librespot#281
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 @Malvineous on GitHub (Feb 23, 2020).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/437
If you leave librespot running for a few days, eventually it crashes:
It looks like the problem is in
channel.rs:12where the sequence is au16, and after a couple of days this overflows causing the error. I tested this by printing the sequence value and it does not reset after each song, getting larger and larger as each song is played.I am tempted to change it to a
u32but it looks like I might have to change thechannelsHashMapto also be au32, which then looks like it could start using up more and more memory.I'm wondering whether, after some time,
sequenceshould be reset to zero andchannelsshould be wiped clean. Or perhaps simply settingsequenceback to zero would work, overwriting the oldHashMapentries?@kaymes commented on GitHub (Feb 26, 2020):
I think it must stay u16 because the id in u16 format is part of the network protocol. Thus we can't change that.
It seems to me that there is something missing in the code: entries only ever get added to the HashMap but never deleted. This results in the HashMap getting saturated with sequential entries. If that's desired, then a HashMap is the wrong data structure and a preallocated array of Options would be more suitable because saturated HashMaps are inefficient.
But the better approach would be to actually detect when a channel is finished and then remove it from the HashMap. There should only be about a hand full of active channels at any given time. If they get removed when obsolete then the HashMap stays nice and small.
@Malvineous commented on GitHub (Feb 26, 2020):
I'm not familiar enough with the code to know whether the entries do get retired from the HashMap or not, but even if they do, the code eventually crashes once the sequence reaches 65536.
I've been running for a few days now with this patch, and the sequence number has wrapped multiple times without any apparent issues, so it seems this fix hasn't broken anything at least and fixed the crash for sure.