Skip to content
This repository was archived by the owner on Jan 20, 2021. It is now read-only.

Fix reset panicking on non-empty queue - #189

Open
jwaataja wants to merge 3 commits into
matthieugrieger:masterfrom
jwaataja:queue-fix
Open

jwaataja wants to merge 3 commits into
matthieugrieger:masterfrom
jwaataja:queue-fix

Conversation

@jwaataja

@jwaataja jwaataja commented Jul 21, 2018

Copy link
Copy Markdown

Before submitting this pull request, please acknowledge that you have done the following:

  • I have tested my changes to make sure that they work
  • I have at least attempted to write relevant unit tests to verify that my changes work
  • I have read through the contribution guidelines
  • I have written any necessary documentation (for example, adding information about a feature to the README)

What type of pull request is this?

  • Bug fix
  • Typo fix
  • New feature implementation
  • New service implementation
  • Other

Description of your pull request:

This fixes a bug where reset panics on any non-empty queue. This was because the PlayCurrent function in queue launched a goroutine that waits until the current track is done playing, and then skips. After calling reset, this routine attempted to skip the current a track, causing an error. I added a check in the Skip function to prevent this crash.

I attempted to write the tests for this, but to actually reproduce the bug I think there has to be a valid audio stream so I just tested it manually. In the process I added some basic tests to the reset command, though none of them test this bug.

jwaataja added 3 commits July 20, 2018 16:39
Fixed an issue where reset didn't work when the queue had any tracks in
it. The issue stemmed from the fact that in the last few lines of the
PlayCurrent method in the queue.go, around line 314, it sets up another
thread to wait and eventually skip the current track. When reset is
called this skip is triggered on an empty queue which causes an error.
Fixed it by adding a check around the offending lines.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant