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

Allow a ConcurrentCircularBuffer to be reinstated after being canceled. #1218

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

paulhoux
Copy link
Collaborator

@paulhoux paulhoux commented Dec 6, 2015

Canceling allows one to signal threads that are waiting for a push or pop operation to simply stop waiting. But without a copy constructor, there was no way to undo the cancellation afterward in order to use the buffer again.

…d. Canceling allows one to signal threads that are waiting for a push or pop operation to simply stop waiting. But without a copy constructor, there was no way to undo the cancellation afterward in order to use the buffer again.
@richardeakin
Copy link
Collaborator

To avoid having to call reinstate(), could we set mCancelled = false from within pushFront()? Right after the lock is taken.

@paulhoux
Copy link
Collaborator Author

paulhoux commented Dec 9, 2015

I don't think that's a waterproof solution, Rich, as the main thread might want to cancel all operations in order to quit the secondary thread that's waiting to pop an item, but then a third thread might push a new item. The main thread would then not be able to terminate and the application would never quit.

@richardeakin
Copy link
Collaborator

I suppose you're right. Pretty messy to have to remember to 'reset' a data structure, but then I guess so are multi-producer / multi-consumer queues in general.

@richardeakin
Copy link
Collaborator

Oh and I do like that you added the bool return, so that the producer knows if he wasn't able to push anything and its not necessarily a failure. I did the same for audio::dsp::RingBuffer's read() and write() methods.

@paulhoux
Copy link
Collaborator Author

Perhaps we can implement the cancel method in a different way, so that it directly notifies all waiting threads to cancel, without using a boolean for that. After the call to cancel, the buffer would be ready to go again without us having to call reinstate. Let me see if I can make that work.

…ts until all threads have canceled their operations, then 'uncancels' itself.
@paulhoux
Copy link
Collaborator Author

I think this is a better solution. The call to cancel now waits until all other threads have canceled and then reinstates the buffer automatically.

@paulhoux
Copy link
Collaborator Author

Here's a Gist that you can use to test it. Just copy-paste it over the BasicApp sample:
https://gist.github.com/paulhoux/fb202c6dcaddc7933687

@richardeakin
Copy link
Collaborator

Thanks for searching for a one-shot solution. I'll defer to Andrew's judgement on this one since he wrote the original thread-safe wrapper, although the first thing that jumps out at me is that you might end up blasting your cpu during the cancel on a big copy because of the while( true ) { sleep( 0 ); } bit. It'd be nice if the conditions could be reused for this, but I'm not sure if that makes any sense in your use case.

@paulhoux
Copy link
Collaborator Author

@richardeakin thanks for your suggestion to use a condition variable instead of a sleep(0). It made the code even cleaner.

@paulhoux
Copy link
Collaborator Author

I've tested this with 32 consumer threads and the main thread acting as a producer thread plus one additional producer thread that pushes a value every 1 milliseconds.

Canceling from the main thread correctly signals all consumers to stop waiting. The call to cancel will be blocking until all consumers have canceled, but instead of spin waiting I now use a condition variable to notify the main thread when all consumers have canceled.

Producers may still (try to) push during cancellation, but their call immediately returns false. Everything works the way it should.

To clarify the scenario in which you'd need functionality like this: imagine having a class that uses a separate thread to do some work. You want to be able to start and stop the thread during the execution of your app. The class uses the ConcurrentCircularBuffer to pass information to the thread. To stop the thread, we call cancel on the buffer, so it stops waiting for input. The thread can then be joined and terminated. But when we want to start the thread again, the buffer would be in canceled state without the modifications in this PR and we would never be able to use it again.

@richardeakin
Copy link
Collaborator

This looks good to me although I haven't tested (but it appears you have extensively!). Just to be clear, the wait here will wait until the next attempted push or pop, which might be indefinitely but this doesn't matter right?

@paulhoux
Copy link
Collaborator Author

You're sharp, dude. I'll do a test to see what happens if there aren't any threads waiting for results.

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

Successfully merging this pull request may close these issues.

3 participants