Skip to content

EventDeduplicator must close its own channel#2488

Open
nluaces wants to merge 4 commits into
skupperproject:mainfrom
nluaces:fix-2485
Open

EventDeduplicator must close its own channel#2488
nluaces wants to merge 4 commits into
skupperproject:mainfrom
nluaces:fix-2485

Conversation

@nluaces

@nluaces nluaces commented Jun 9, 2026

Copy link
Copy Markdown
Member

Fixes #2485

@nluaces nluaces self-assigned this Jun 9, 2026
@nluaces nluaces added the bug Something isn't working label Jun 9, 2026

@c-kruse c-kruse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that this introduces a different chance for a panic: QueueEvent after Stop tries to send on a closed eventCh channel.

Rather than trying to guard QueueEvent or something, I think we'd be better off with a new done channel owned by the EventDeduplicator to clean up goroutine(s) when Stop() is called instead of giving eventCh a second purpose.

As an aside, I'm not sure there's much reason to split out a new goroutine to listen for stopCh. It may end up cleaner to keep everything in processEvents.

@nluaces

nluaces commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

@c-kruse I see your point. Regarding what you have said here

As an aside, I'm not sure there's much reason to split out a new goroutine to listen for stopCh. It may end up cleaner to keep everything in processEvents.

The reason of the splitting was that during testing, after queuing a new event and stopping the channel, proccesEvents was choosing randomly to process the event rather than stopping its channel most of the time.

I will try what you have suggested.

@nluaces nluaces requested a review from c-kruse June 11, 2026 15:46

@c-kruse c-kruse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

Comment thread internal/nonkube/controller/event_deduplicator_test.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System controller crashing on site delete (using reload-type: auto)

2 participants