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

Make isempty(c::Channel) a non-mutating operation #36641

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

staticfloat
Copy link
Member

Previously, isempty(c::Channel) would fall back to iterate(c) === nothing, which actually consumed a value from the channel. Instead,
let's just define it in terms of isready(c) since that already exists.

@StefanKarpinski StefanKarpinski requested a review from vtjnash July 13, 2020 18:57
@StefanKarpinski
Copy link
Member

Seems like arguably a bugfix. I would not think that isempty should consume anything in general.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I wonder if we should also define this in terms of isempty on the internal objects (O(1)) instead of length (O(n)), but this is a good fix regardless

@staticfloat
Copy link
Member Author

I'm honestly not sure when this fallback is a good idea:

iterate(itr) === nothing

It seems a bad idea to me to ever call iterate(itr) to check if it's empty.

@ararslan ararslan added bugfix This change fixes an existing bug needs tests Unit tests are required for this change labels Jul 13, 2020
Previously, `isempty(c::Channel)` would fall back to `iterate(c) ===
nothing`, which actually consumed a value from the channel.  Instead,
let's just define it in terms of its internal datastructures.
@staticfloat staticfloat removed the needs tests Unit tests are required for this change label Jul 13, 2020
@Keno
Copy link
Member

Keno commented Jul 13, 2020

I'm honestly not sure when this fallback is a good idea:

We have very few iterators that are mutable, but they have a few extra requirements. We should probably document what those are ... or at least figure out what they are ;).

@Keno
Copy link
Member

Keno commented Jul 13, 2020

That said, there is a bit of a problem with this kind of query in general, since isempty being true here doesn't mean that you can't get an object from it, it just means that it'll block. Obviously this definition is strictly better than the fallback we had before (since that answer would be both useless and maybe drop an object you care about), but I'm wondering if we should have it at all.

@staticfloat
Copy link
Member Author

since isempty being true here doesn't mean that you can't get an object from it, it just means that it'll block

I think that's the difference between isempty() and !isopen(); just because something is empty doesn't mean it's permanently empty.

@JeffBezanson
Copy link
Member

I am quite sure we did not intend isempty to be used on Channels, since it doesn't really make sense to have both that and isready. I also wish isready did not exist, since it's just a race condition. So I'm curious, how did this come up?

@staticfloat
Copy link
Member Author

how did this come up?

The usual method of applying verbs from other areas and seeing if they work; I had a Channel that I closed and wanted to drain, so I wrote:

close(c)
while !isempty(c)
    process(take!(c))
end

Which seemed to work, except then I noticed it was only processing every other item. I would be fine with defining isempty() more narrowly and telling everyone to use isready(). I agree that there's something of a mixture of concerns here; what my use case really wants is for eof(::Channel) to be defined, which would be something like !isopen(c) && !isready(c). That way we aren't relying upon the caller to have already closed the channel to avoid raciness.

@JeffBezanson
Copy link
Member

I think the only thing that really works for that is something like iterate, where you either get a value or find out there aren't any more.

@codecov-commenter
Copy link

Codecov Report

Merging #36641 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36641   +/-   ##
=======================================
  Coverage   86.15%   86.15%           
=======================================
  Files         349      349           
  Lines       65809    65809           
=======================================
  Hits        56698    56698           
  Misses       9111     9111           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 110765a...5ad48c7. Read the comment docs.

@JeffBezanson JeffBezanson merged commit 4481500 into master Jul 15, 2020
@JeffBezanson JeffBezanson deleted the sf/isempty_channel branch July 15, 2020 19:32
JeffBezanson pushed a commit that referenced this pull request Jul 21, 2020
Previously, `isempty(c::Channel)` would fall back to `iterate(c) ===
nothing`, which actually consumed a value from the channel.  Instead,
let's just define it in terms of its internal datastructures.

(cherry picked from commit 4481500)
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
Previously, `isempty(c::Channel)` would fall back to `iterate(c) ===
nothing`, which actually consumed a value from the channel.  Instead,
let's just define it in terms of its internal datastructures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants