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

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

Closed
wants to merge 2 commits into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Oct 29, 2019

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
Contributor

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

@jimmo
Copy link
Member Author

jimmo commented Oct 29, 2019

Updated tests to catch the missing coverage.

@dpgeorge
Copy link
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.

raised CancelledError()
None
CancelledError()
raised ValueError('exception',)
Copy link
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
Member Author

Choose a reason for hiding this comment

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

Done

@kevinkk525
Copy link
Contributor

kevinkk525 commented Oct 30, 2019

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
Copy link
Member Author

jimmo commented Oct 30, 2019

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

mp_raise_TypeError("can't pend throw to just-started generator");
if (exc_in == mp_const_none) {
mp_raise_ValueError(NULL);
}
Copy link
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
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
@dpgeorge
Copy link
Member

dpgeorge commented Nov 4, 2019

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
Copy link
Member

dpgeorge commented Nov 4, 2019

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants