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

Check channel state on iterate #52981

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jakobnissen
Copy link
Contributor

Before, iterate on Channel would return nothing when the channel was closed, not checking if it was closed with an error.

Closes #52974

@jakobnissen jakobnissen changed the title Check task state on iterate Check channel state on iterate Jan 19, 2024
@jakobnissen
Copy link
Contributor Author

Okay that implementation won't work, and a real implementation will have a bunch of atomic and concurrency stuff I don't know enough about to be confident in trying, and won't be confident that I won't create hard-to-detect bugs

base/channels.jl Outdated
@@ -619,6 +619,7 @@ function iterate(c::Channel, state=nothing)
end
end
else
check_channel_state(c)
Copy link
Member

@vtjnash vtjnash Jan 19, 2024

Choose a reason for hiding this comment

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

You could inline this implementation but skip throwing the InvalidStateException, or perhaps more easily, just delete the isopen check at the top and let take! handle everything (as it already implements everything necessary)

@jakobnissen
Copy link
Contributor Author

Test failures appear to be spurious libgit connection issues.

I think this PR is good to go, modulo one decision: This PR's current behaviour is that iterate on a closed channel will not throw if the channel is not empty. The docs for close states that this is the case for take! and fetch, but does not mention iterate. Is this correct? Could the docs for close be expanded to mention iterate?

@jakobnissen jakobnissen added multithreading Base.Threads and related functionality bugfix This change fixes an existing bug iteration Involves iteration or the iteration protocol awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. labels May 25, 2024
@jakobnissen
Copy link
Contributor Author

Bump

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

I like the change/fix/clean up here; I've been annoyed before that it always throws when done iterating.

Before, iterate on Channel would return `nothing` when the channel was closed,
not checking if it was closed with an error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. bugfix This change fixes an existing bug iteration Involves iteration or the iteration protocol multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iterate on Channel fails to throw errors, against docs
3 participants