Skip to content

Notify all when RWQueue stops #2775

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

Merged
merged 1 commit into from
Aug 20, 2023
Merged

Notify all when RWQueue stops #2775

merged 1 commit into from
Aug 20, 2023

Conversation

weirddan455
Copy link
Collaborator

Found another bug in RWQueue. We need to notify all threads when the queue stops or we can have a deadlock.

I'm pretty sure I'll need to add a couple more methods to this class as well once I get FFmpeg threading sorted out but that'll go in my big PR alongside the use case. At a minimum, I think I'll need a Drain method and a Start method so that I can Stop the queue, Drain all remaining elements from it in a non-blocking manner, and then Start it back up again.

For now though, I'm just pushing these bug fixes in small PRs as I see them since this could cause a stall in existing code.

@weirddan455 weirddan455 requested review from johnnovak and kcgen August 20, 2023 01:13
@kcgen
Copy link
Member

kcgen commented Aug 20, 2023

Looks good 👍
For draining, check out the other uses in the midi code (how we drain the queue on shutdown), which hopefully can be used.

@kcgen kcgen added the plumbing Issues related to low-level support functions and classes label Aug 20, 2023
@kcgen kcgen merged commit 043aed0 into main Aug 20, 2023
@kcgen
Copy link
Member

kcgen commented Aug 20, 2023

In the main thread, call .Stop() on the queue, after which wait for the thread to become joinable.

Then inside the threaded processing function, always check the .IsRunning() state on the queue after each unit of work. We usually have it as a while-condition that bails out once it's false.

git grep IsRunning can find the chunks where we do this in the capture and midi code.

Hope it helps!

@weirddan455 weirddan455 deleted the wd/rwqueue-notify branch August 20, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plumbing Issues related to low-level support functions and classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants