libartifact: add CloseEventChannel to cleanly stop event streams#831
libartifact: add CloseEventChannel to cleanly stop event streams#831nimdrak wants to merge 1 commit into
Conversation
| ch := as.EventChannel() | ||
| defer func() { | ||
| as.CloseEventChannel() | ||
| }() |
There was a problem hiding this comment.
if you want to add an actual test because this was already passing you need to add extra reads from the channel to ensure this is not blocking after you close it
There was a problem hiding this comment.
Thanks for the review. I've addressed your feedback, squashed the changes into the original commit, and force-pushed. Please check it here 03c5b33
| func (as *ArtifactStore) CloseEventChannel() { | ||
| if as.eventChannel != nil { | ||
| close(as.eventChannel) | ||
| } | ||
| } |
There was a problem hiding this comment.
This and well even EventChannel() are not safe for concurrent use. Another goroutine might as well try to write to it after this is closed.
Generally design wise nothing prevents users from keep using the store after this is closed which will cause panics. At the very least as.eventChannel would need to be set to nil but even that will not prevent any race conditions.
Overall the more I look at this and libimage event handling the more I see how totally broken (racy) this all is, I really need to stress that we need something like #465 (comment) instead which would avoid basically all race conditions. OF course out of scope for this I guess...
At the minimum I would expect big comments here warning of any concurrent use.
There was a problem hiding this comment.
I see your point. Since you mentioned fixing the race condition is out of scope for this PR, I will add the warning comment as you suggested. I agree that it would be better if the architecture becomes robust towards concurrent access in the future.
Out of curiosity, I initially thought the artifact store would only be accessed by a single goroutine from a single libpod runtime at a time. Are there actual use cases where it is handled concurrently? I would highly appreciate it if you could share your insights into the broader architecture so I can understand it better.
There was a problem hiding this comment.
@Luap99 should we make an issue for your proposal long-term?
nimdrak
force-pushed
the
add_shutdown_eventchannel_in_libartifact
branch
from
c4ba785 to
03c5b33
Compare
|
@nimdrak please force push, cirrus CI was not triggered right here |
- Add CloseEventChannel with a warning comment to `CloseEventChannel` regarding unsafe concurrent usage - Refactor `TestArtifactStore_EventChannel` to isolate test setups per `t.Run`. - Add a dedicated test case to verify that `CloseEventChannel` properly closes the channel and ensures non-blocking reads. Signed-off-by: Byounguk Lee <nimdrak@gmail.com>
nimdrak
force-pushed
the
add_shutdown_eventchannel_in_libartifact
branch
from
03c5b33 to
ecc9631
Compare
|
@Luap99 I did. Thanks for your review! |
3 participants
Fixes #465
We need a way to cleanly close the event channel from the sender side to prevent resource leaks.
This requirement was identified while implementing artifact event handling in Podman (containers/podman#28662).