search

andreiavrammsd / cpp-channel

Thread-safe container for sharing data between threads
https://blog.andreiavram.ro/cpp-channel-thread-safe-container-share-data-threads/
MIT License
400 stars 28 forks source link

Fix: Close notify all consumers #17

Closed lbb closed 2 years ago

lbb commented 2 years ago

Hi,

we have multiple consumers listening on the same channel. When the channel is closed only one consumer is notified. This PR addresses this issue by notifying all listening threads on close().

lbb commented 2 years ago

Maybe this s a wrong observation, but why are we notifying before updating the value of is_closed? Does this not pose a race condition?

andreiavrammsd commented 2 years ago

Hi, @lbb,

Many thanks for trying the library!

Indeed, notify_all is needed. The examples serve as some basic integration tests. Could you please add your use case to a new example file (eg: close_with_multiple_consumers.cpp) or update the existing close example?

If the channel is closed, you cannot write into it anymore. You can only read from it. If you only read from it, there should be no race. Did you notice something different? But from a logical point of view, it might be a better approach to first mark it as closed and only then to notify consumers.

andreiavrammsd commented 2 years ago

Will you be able to add/update the close example?

lbb commented 2 years ago

@andreiavrammsd I encountered double more problems with multiple consumers. This happens when multiple consumers are trying to remove elements from the ch.queue.

andreiavrammsd commented 2 years ago

Could you please write an example that reproduces the issue?

andreiavrammsd commented 2 years ago

@lbb, I did some tests and fixes (including the ones you provided). Can you test this branch?

andreiavrammsd commented 2 years ago

I included your fix, besides other improvements, in #18. Thank you for the input!