Skip to content

py/objgenerator.c: Allow pend_throw to an unstarted generator. - #5275

Closed
jimmo wants to merge 2 commits into
micropython:masterfrom
jimmo:generator-pend-throw
Closed

jimmo wants to merge 2 commits into
micropython:masterfrom
jimmo:generator-pend-throw

Conversation

@jimmo

@jimmo jimmo commented Oct 29, 2019

Copy link
Copy Markdown
Member

pend_throw is a MicroPython-specific extension but allows simplification of the implementation of uasyncio. It injects an exception into the generator such that it will be raised as soon as it is next resumed.

This fixes the behaviour specifically such that uasyncio.cancel() will work on an unstarted generator. See #5242

This wasn't necessary as the wrapped function already has a reference
to its globals. But it had a dual purpose of tracking whether the
function was currently running, so replace it with a bool.
@kevinkk525

Copy link
Copy Markdown
Contributor

I just tested the behaviour and can confirm that it works as expected. Thanks.

@jimmo
jimmo force-pushed the generator-pend-throw branch from 78b61ff to dcbdfba Compare October 29, 2019 23:19
@jimmo

jimmo commented Oct 29, 2019

Copy link
Copy Markdown
Member Author

Updated tests to catch the missing coverage.

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Oct 30, 2019
@dpgeorge

Copy link
Copy Markdown
Member

Overall code-size change for this PR is:

   bare-arm:   -24 -0.036% 
minimal x86:   -12 -0.008% 
   unix x64:   -48 -0.010% 
unix nanbox:    -8 -0.002% 
      stm32:   -20 -0.006% PYBV10
     cc3200:   -32 -0.017% 
    esp8266:   -44 -0.007% GENERIC
      esp32:   -32 -0.003% GENERIC[incl -32(data)]
        nrf:   +24 +0.016% pca10040
       samd:   -32 -0.031% ADAFRUIT_ITSYBITSY_M4_EXPRESS

So, independently of whether this is eventually useful for uasyncio, I'd say it's a good change because 1) it generalises a feature; 2) it reduces code size.

@jimmo
jimmo force-pushed the generator-pend-throw branch from dcbdfba to 70154ec Compare October 30, 2019 01:20

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

now needs to be replaced with raised ValueError()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

@jimmo
jimmo force-pushed the generator-pend-throw branch from 70154ec to ef209b1 Compare October 30, 2019 05:00
@kevinkk525

kevinkk525 commented Oct 30, 2019 •

Copy link
Copy Markdown
Contributor

A quick question to your PR:
I saw an attribute "pend_exc" that was "is_running" before but the purpose is not entirely clear to me: Does it indicate the generators general status? Like will it be running until it is fully cancelled or does it only indicate that the generator is currently executing some code until it yields?

If it indicates the general status of a generator, could it be used to implement the CPython equivalent of "task.done()" returning if the generator is finished executing code?
This way we would be able to check if a generator is fully canceled/finished without having a task implementation.
If that is not the case and the attribute is used differently, you don't need to explain it, my understanding is not important in this case :D I just thought about "coro.done()" when I saw it.

@jimmo

jimmo commented Oct 30, 2019

Copy link
Copy Markdown
Member Author

I saw an attribute "pend_exc" that was "is_running" before but the purpose is not entirely clear to me: Does it indicate the generators general status? Like will it be running until it is fully cancelled or does it only indicate that the generator is currently executing some code until it yields?

It's the latter. i.e. the generator is currently executing code up to the next yield. This really only exists to prevent trying to call next/send/throw (or pend_throw) on itself. (See the test case I added that shows this in action).

If it indicates the general status of a generator, could it be used to implement the CPython equivalent of "task.done()" returning if the generator is finished executing code?
This way we would be able to check if a generator is fully canceled/finished without having a task implementation.

Yeah, unfortunately it doesn't, but it could actually be easily extended to do so with another sentinel value (this is actually what I had in my mind when I wrote "another extension to objgenerator.c to interrogate a generator instance and find out whether it has finished" on #5242

Comment thread py/objgenerator.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this restriction needed? Can this check just be removed? If the user passes in None then it just cancels any pending throw, which seems sensible/possible...?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. Removed and updated the test to check that this works.

I think this was left over from when I had the meaning of MP_OBJ_NULL and mp_const_none the other way around.

Replace the is_running field with a tri-state variable to indicate
running/not-running/pending-exception.

Update tests to cover the various cases.

This allows cancellation in uasyncio even if the coroutine hasn't
been executed yet. Fixes micropython#5242
@jimmo
jimmo force-pushed the generator-pend-throw branch from ef209b1 to fdaa191 Compare November 4, 2019 04:34
@dpgeorge

dpgeorge commented Nov 4, 2019

Copy link
Copy Markdown
Member

With the latest changes/fixes to this PR the code-size change is:

   bare-arm:   -48 -0.072% 
minimal x86:   -60 -0.040% 
   unix x64:   -80 -0.016% 
unix nanbox:   -40 -0.009% 
      stm32:   -36 -0.010% PYBV10
     cc3200:   -40 -0.022% 
    esp8266:   -60 -0.009% GENERIC
      esp32:   -64 -0.005% GENERIC[incl -48(data)]
        nrf:   -36 -0.025% pca10040
       samd:   -48 -0.047% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@dpgeorge

dpgeorge commented Nov 4, 2019

Copy link
Copy Markdown
Member

Merged in 576ed89 and 5578182

@dpgeorge dpgeorge closed this Nov 4, 2019
tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 31, 2021
…y-headers

Fix crash in gen_display_resources.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

py-core Relates to py/ directory in source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants