Skip to content

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Nov 23, 2023

Currently, stream.Debounce can emit a spurious event when the source stream terminates. Indeed, at that point, the items channel gets closed, and the completion error is published to a separate error channel. Depending on the (undeterministic) select unblocking order, we may first observe the items channel closure, hence emitting the default value of the object, and only afterwards propagate the termination error.

Let's fix this issue by checking whether the items channel unblocked because it had just been closed, and do not emit any item in that case. Additionally, let's also slightly adapt the corresponding test to catch this issue: we never discovered it because another event had just been emitted, and the spurious one was just queued. Adding an extra delay, instead, causes the test (without this fix) to almost always hang, because of the spurious event not being received by anyone.

Additionally, let's also prevent entering into a busy loop when the items channel gets closed, until we receive on the errors channel, by immediately setting it to nil afterwards. Thanks to David Bimmler for pointing out this issue.

@giorio94 giorio94 added release-note/misc This PR makes changes that have no direct user impact. area/agent Cilium agent related. labels Nov 23, 2023
@giorio94 giorio94 requested a review from a team as a code owner November 23, 2023 09:25
@giorio94 giorio94 requested a review from rgo3 November 23, 2023 09:26
Currently, stream.Debounce can emit a spurious event when the source stream
terminates. Indeed, at that point, the items channel gets closed, and the
completion error is published to a separate error channel. Depending on the
(undeterministic) select unblocking order, we may first observe the items
channel closure, hence emitting the default value of the object, and only
afterwards propagate the termination error.

Let's fix this issue by checking whether the items channel unblocked
because it had just been closed, and do not emit any item in that case.
Additionally, let's also slightly adapt the corresponding test to catch
this issue: we never discovered it because another event had just been
emitted, and the spurious one was just queued. Adding an extra delay,
instead, causes the test (without this fix) to almost always hang,
because of the spurious event not being received by anyone.

Additionally, let's also prevent entering into a busy loop when the
items channel gets closed, until we receive on the errors channel,
by immediately setting it to nil afterwards. Thanks to David Bimmler
for pointing out this issue.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 force-pushed the mio/stream-debounce-fix branch from 72b1c62 to aa2d71a Compare November 23, 2023 09:45
@giorio94
Copy link
Member Author

/test

@giorio94 giorio94 requested a review from bimmlerd November 23, 2023 09:49
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

Nice catch by @bimmlerd

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 23, 2023
@tklauser tklauser added this pull request to the merge queue Nov 24, 2023
Merged via the queue into cilium:main with commit 2b413c3 Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Cilium agent related. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants