[GH-ISSUE #437] Crash after running for some time - sequence overflow #281

Closed
opened 2026-02-27 19:29:48 +03:00 by kerem · 2 comments
Owner

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:

[DEBUG librespot_audio::fetch] Read at postion 5317412 completed. 4096 bytes returned, 8192 bytes were requested.
thread 'main' panicked at 'attempt to add with overflow', core/src/util/mod.rs:46:1
stack backtrace:
   8:     0x557d1718ffea - core::panicking::panic::hb62b3816d7e34515
   9:     0x557d166efd28 - <u16 as librespot_core::util::Seq>::next::hc565224adbe232c0
                               at core/src/util/mod.rs:41
  10:     0x557d16713784 - librespot_core::util::SeqGenerator<T>::get::hf5cb4433501972f5
                               at core/src/util/mod.rs:57
  11:     0x557d166da2e0 - librespot_core::channel::ChannelManager::allocate::{{closure}}::h6ea19c59c3ca7791
                               at core/src/channel.rs:49
  12:     0x557d166dab1c - librespot_core::channel::ChannelManager::lock::h7c8c9fc481f35d82
                               at core/src/component.rs:18
  13:     0x557d167437ed - librespot_core::channel::ChannelManager::allocate::h2d4e9f8b669384f0
                               at core/src/channel.rs:48
  14:     0x557d165d8ad3 - librespot_audio::fetch::request_range::hf59af9d826a9ad95
                               at audio/src/fetch.rs:467

It looks like the problem is in channel.rs:12 where the sequence is a u16, 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 u32 but it looks like I might have to change the channels HashMap to also be a u32, which then looks like it could start using up more and more memory.

I'm wondering whether, after some time, sequence should be reset to zero and channels should be wiped clean. Or perhaps simply setting sequence back to zero would work, overwriting the old HashMap entries?

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: ``` [DEBUG librespot_audio::fetch] Read at postion 5317412 completed. 4096 bytes returned, 8192 bytes were requested. thread 'main' panicked at 'attempt to add with overflow', core/src/util/mod.rs:46:1 stack backtrace: 8: 0x557d1718ffea - core::panicking::panic::hb62b3816d7e34515 9: 0x557d166efd28 - <u16 as librespot_core::util::Seq>::next::hc565224adbe232c0 at core/src/util/mod.rs:41 10: 0x557d16713784 - librespot_core::util::SeqGenerator<T>::get::hf5cb4433501972f5 at core/src/util/mod.rs:57 11: 0x557d166da2e0 - librespot_core::channel::ChannelManager::allocate::{{closure}}::h6ea19c59c3ca7791 at core/src/channel.rs:49 12: 0x557d166dab1c - librespot_core::channel::ChannelManager::lock::h7c8c9fc481f35d82 at core/src/component.rs:18 13: 0x557d167437ed - librespot_core::channel::ChannelManager::allocate::h2d4e9f8b669384f0 at core/src/channel.rs:48 14: 0x557d165d8ad3 - librespot_audio::fetch::request_range::hf59af9d826a9ad95 at audio/src/fetch.rs:467 ``` It looks like the problem is in `channel.rs:12` where the sequence is a `u16`, 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 `u32` but it looks like I might have to change the `channels` `HashMap` to also be a `u32`, which then looks like it could start using up more and more memory. I'm wondering whether, after some time, `sequence` should be reset to zero and `channels` should be wiped clean. Or perhaps simply setting `sequence` back to zero would work, overwriting the old `HashMap` entries?
kerem closed this issue 2026-02-27 19:29:48 +03:00
Author
Owner

@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.

<!-- gh-comment-id:591661791 --> @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.
Author
Owner

@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.

<!-- gh-comment-id:591676398 --> @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.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/librespot#281
No description provided.