Ensure regen utility class gets marked as done when concretised

[maybe-sybr]

Description

Ensure regen() instances get marked as done when concretised via the
UserList.data property implementation. We also improve laziness of
regen.__repr__() so it reflects the fact that it's lazy.

Fixes: #6786

[codecov]

Codecov Report

Merging #6789 (d419012) into master (799f839) will increase coverage by 0.00%.
The diff coverage is 85.71%.

❗ Current head d419012 differs from pull request most recent head 64da3e8. Consider uploading reports for the commit 64da3e8 to get more accurate results

@@           Coverage Diff           @@
##           master    #6789   +/-   ##
=======================================
  Coverage   70.69%   70.70%           
=======================================
  Files         138      138           
  Lines       16602    16601    -1     
  Branches     2091     2093    +2     
=======================================
  Hits        11737    11737           
+ Misses       4669     4668    -1     
  Partials      196      196           

Flag	Coverage Δ
unittests	70.70% <85.71%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
celery/utils/functional.py	70.51% <85.71%> (+0.44%)	⬆️

---

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 799f839...64da3e8. Read the comment docs.

[maybe-sybr]

This test is for an edge case where a repeatable iterator could raise an exception at some point before completion which would result in regen() stashing the elements it saw but never marking itself as done. Then the next time we yield elements from an iterator derived from the regen() instance, we'd yield the stashed elements, then repeat them from the underlying iterator if it's capable of giving them to us again.

IDK if this is something we can even bother handling. If the underlying iterator explodes, we could choose to mark the regen instance as __done and just give up on attempting to get more elements? If we want to go down that route, I can roll a change into this PR since it'd be pretty simple and still thematically relevant. If we think we need to do something more involved or optimistic, I'll make a follow up issue for discussion.

[maybe-sybr]

I've made #6794 to track the discussion on this point. Could just be a thing we need to document, happy to have this PR merged without addressing that issue yet.

[lgtm-com]

This pull request fixes 1 alert when merging 42685f2 into c93371d - view on LGTM.com

fixed alerts:

• 1 for Non-exception in 'except' clause

[maybe-sybr]

Why do we have except StopIteration here at all? list() should handle that.

Good question. That was added in 9982773 and looks like it could have been deliberate, but I have no idea why that might be the case. AFAICT you'd have to be raising StopIteration from inside a generator (which is wrong) to need something like that :/ A quick sanity check of list(iter(range(5))) works without raising and, as expected, spits out StopIteration when I next() the iterator manually. Not sure what the point is there.

Removing the try: except StopIteration: doesn't seem to break any unit or integration tests (against redis), so I'll amend the diff and re-push shortly.

[lgtm-com]

This pull request introduces 2 alerts when merging 9852391 into b0ebc3b - view on LGTM.com

new alerts:

• 1 for Non-exception in 'except' clause
• 1 for Module is imported with 'import' and 'import from'

[Suor]

AFAICT you'd have to be raising StopIteration from inside a generator (which is wrong) to need something like that

Since Python 3.7 raising StopIteration inside generator results in a RuntimeError.

[thedrow]

AFAICT you'd have to be raising StopIteration from inside a generator (which is wrong) to need something like that

Since Python 3.7 raising StopIteration inside generator results in a RuntimeError.

This is probably an oversight we made during the Python 3 only migration in 5.0.
Thanks for noticing 😄

[maybe-sybr]

I just pushed a change which removed what I thought was a pointless iter(self) but I now realise that it was to ensure that the __done attribute gets set when the underlying iterator is completed without having to do hard stuff in the __getitem__() method. I've stripped that commit out, but I still think it's wasteful to spend time iterating on the (potentially many) initial elements of ourself just to get some subsequent element from the underlying iterator.

Edit: I spent some more time on this and have an improved implementation but some of the tests aren't playing nice with it. For the moment, I'm going to call this changeset done and ready for a review prior to merge

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 64da3e8 into 799f839 - view on LGTM.com

new alerts:

• 1 for Non-exception in 'except' clause

fixed alerts:

• 1 for Module is imported with 'import' and 'import from'

[Suor]

I just pushed a change which removed what I thought was a pointless iter(self) but I now realise that it was to ensure that the __done attribute gets set when the underlying iterator is completed without having to do hard stuff in the getitem() method. I've stripped that commit out, but I still think it's wasteful to spend time iterating on the (potentially many) initial elements of ourself just to get some subsequent element from the underlying iterator.

I wonder how much use that .__getitem__() call gets. At least for .map() and friends we simply serialize, i.e. convert to list the thing eventually, we only need regen() to not consume immediately, since a signature might never get sent.

[maybe-sybr]

I wonder how much use that .__getitem__() call gets. At least for .map() and friends we simply serialize, i.e. convert to list the thing eventually, we only need regen() to not consume immediately, since a signature might never get sent.

I'd guess the only time it really gets called is to access the 0th or -1th item which are used for various things in the canvas code. Otherwise I'd expect it to be mostly simple for x in some_regen_thing: usage.

[auvipy]

let's merge it after some 5.1.x point releases

[thedrow]

let's merge it after some 5.1.x point releases

I think we can include it in 5.1.1 unless there's a special reason not to do so that I missed.

[auvipy]

OK. merged as bugfix