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

chann: support to consume all data after Close the channel. #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

amyangfei
Copy link

ref: #3

In order to fix this new bug: #3 (comment), this PR removes the default branch when sending backlog data into output channel.

The drawback is when there is no receiver or the receiver doesn't consume all data that has been sent to input channel, the unboundedProcessing goroutine will leak forever. IMO the user of this library should take care of this behavior.

Considering the channel behavior in standard library, if user doesn't receive data from a buffered channel, data in the buffered channel is still kept in memory and has memory leak too, even if the channel is closed.

Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I have several questions

chann.go Outdated Show resolved Hide resolved
chann.go Outdated
// first case will ways be selected. See #3.
default:
ch.q[0] = nilT // de-reference earlier to help GC
ch.q = ch.q[1:]
Copy link
Member

Choose a reason for hiding this comment

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

This reads like a previous implementation that was reported to be problematic, as discussed in #3. Could you help me to understand how your changes can improve the overall situation?

Copy link
Author

@amyangfei amyangfei Jan 10, 2023

Choose a reason for hiding this comment

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

Yep, in this PR, it behaves as follows
When an unbounded channel is closed

// Note if receiver doesn't consume all data that has been sent to input
// channel, the `unboundedProcessing` goroutine will leak forever.
// Ref: https://github.com/golang-design/chann/issues/3
ch.out <- ch.q[0]
Copy link
Member

Choose a reason for hiding this comment

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

As you documented, if there is no receiver, we will leak a goroutine, and it seems not ideal. Isn't it? Or at least do we need to inform the users in the documentation?

Choose a reason for hiding this comment

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

Hi @amyangfei can you take a look at this comment? I want to use this library soon

And as for

Considering the channel behavior in standard library, if user doesn't receive data from a buffered channel, data in the buffered channel is still kept in memory and has memory leak too, even if the channel is closed.
GC should be able to recycle the unreachable objects when we use built-in channel

Or maybe we can add another function NewUnbounded[T]() (*Chann[T], func()) that returns a cleanup function? like context's cancel function, it will remind the user that don't forget to invoke the function.

karolba added a commit to karolba/gparallel that referenced this pull request Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants