Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes a race condition when loading tracks via AutoDJ #4267

Merged
merged 3 commits into from
Sep 8, 2021

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Sep 7, 2021

This fixes https://bugs.launchpad.net/mixxx/+bug/1941743 at two places.
See commits.

@Holzhaus
Copy link
Member

Holzhaus commented Sep 7, 2021

Could you add tests for that issue to avoid future regressions?

@uklotzde uklotzde added this to the 2.3.1 milestone Sep 7, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Sep 7, 2021

Does this fix https://bugs.launchpad.net/mixxx/+bug/1941989 ?

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Does this fix https://bugs.launchpad.net/mixxx/+bug/1941989 ?

Probably not. The skipped tracks are not supposed to have a very short or zero duration.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

Running the manual test from the bug report triggers debug assertions in main due to invalid frame positions. Did you run the tests for 2.3 successfully?

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

I'll publish a PR for main to fix those assertions. Stress testing AutoDJ while constantly cycling through those empty tracks does not show a memory leak.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2021

@daschuer Please run this test for 2.3 and watch the memory footprint.

@daschuer
Copy link
Member Author

daschuer commented Sep 8, 2021

Did you run the tests for 2.3 successfully?

Yes.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one of the race conditions is fixed. While testing I noticed that AutoDJ always loads tracks into the same deck again and again. But this is an edge case and only happens with empty tracks.

Let's merge to fix the bug, even though we don't have any regression tests.

Thank you! LGTM

@uklotzde uklotzde merged commit e55fbd0 into mixxxdj:2.3 Sep 8, 2021
@daschuer
Copy link
Member Author

daschuer commented Sep 8, 2021

While testing I noticed that AutoDJ always loads tracks into the same deck again and again

This is the desired behavior. The AutoDJ loads new tracks until it finds a suitable successor. This happens normally while the previous track is still playing in the other deck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants