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.

Sign up for GitHub

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

Jump to bottom

rp2: KeyboardInterrupt thrown in 2nd thread instead of main #7026

Closed
kevinkk525 opened this issue Mar 13, 2021 · 7 comments
Closed

rp2: KeyboardInterrupt thrown in 2nd thread instead of main #7026

kevinkk525 opened this issue Mar 13, 2021 · 7 comments
Labels
port-rp2

Comments

@kevinkk525
Copy link
Contributor

kevinkk525 commented Mar 13, 2021 •
edited
Loading

I remember having read about it somewhere but couldn't find it before opening this issue..

When using a thread on the pico, my KeyboardInterrupt is always thrown in that thread running on the 2nd core. This is problematic as it always stops my background library instead of whatever I just executed in the repl. The 2nd KeyboardInterrupt will get thrown in my repl on the 1st core but then I always have to reset the device or start the thread again.
In a running application this might not be a problem but for testing it's a nuisance.
Is there any easy way to change this behaviour? I'd say the thread on the 2nd core doesn't even need to catch a KeyboardInterrupt (or at least be the 2nd one to receive it so the repl/1st core comes first).
@dpgeorge
Copy link
Member

I'm not exactly certain, but I think CPython enforces that all signals are to be handled by the main thread, including KeyboardInterrupt. That's obviously not happening in MicroPython so should be fixed.

@kevinkk525
Copy link
Contributor Author

https://docs.python.org/3.9/library/signal.html#signals-and-threads

Python signal handlers are always executed in the main Python thread of the main interpreter, even if the signal was received in another thread.

You're right.

@dpgeorge
Copy link
Member

dpgeorge commented May 8, 2021

Python signal handlers are always executed in the main Python thread of the main interpreter, even if the signal was received in another thread.

That's apparently only true if the signal module is available. The caveats at the end of https://docs.python.org/3/library/_thread.html say this:

Threads interact strangely with interrupts: the KeyboardInterrupt exception will be received by an arbitrary thread. (When the signal module is available, interrupts always go to the main thread.)

@kevinkk525
Copy link
Contributor Author

oh I didn't read that, should have thought about reading the low-level library. Afaik signal is not available on micropython and from the looks of it, it doesn't offer much for micropython either.
So what's the conclusion?
If it's easily achievable, processing the KeyboardInterrupt in the main thread would make development a lot easier, even if it's not completely CPython conform without the signal module.

@dlech
Copy link
Sponsor Contributor

dlech commented May 10, 2021 •
edited
Loading

I agree that for MicroPython it is nicer to have the slightly more deterministic option of always having the KeyboardInterrupt on the main thread.

I implemented this a while back with pybricks@4d22401 and pybricks@bed7a30.

If this looks useful, I can make a PR (it needs to be updated for new ports).

@dpgeorge
Copy link
Member

If this looks useful, I can make a PR (it needs to be updated for new ports).

@dlech Yes that does look good, please make a PR if you can.

dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
482e897 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
b2df361 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
@dlech dlech mentioned this issue May 11, 2021
Closed
dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
b28f83f 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
Signed-off-by: David Lechner <david@pybricks.com>
dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
fd1f82d 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
Signed-off-by: David Lechner <david@pybricks.com>
dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
ce3d730 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
Signed-off-by: David Lechner <david@pybricks.com>
dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
791c6b7 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
Signed-off-by: David Lechner <david@pybricks.com>
dlech added a commit to dlech/micropython that referenced this issue May 11, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
7ff789f 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
Signed-off-by: David Lechner <david@pybricks.com>
dpgeorge pushed a commit that referenced this issue Jun 18, 2021
@dlech @dpgeorge
py/mpstate: Schedule KeyboardInterrupt on main thread.
259d9b6 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Fixes issue #7026.

Signed-off-by: David Lechner <david@pybricks.com>
@dpgeorge
Copy link
Member

Should be fixed by 259d9b6

dlech added a commit to pybricks/micropython that referenced this issue Jun 24, 2021
@dlech
py/mpstate: Schedule KeyboardInterrupt on main thread.
43483fd 
This introduces a new macro to get the main thread and uses it to ensure
that asynchronous exceptions such as KeyboardInterrupt (CTRL+C) are only
scheduled on the main thread. This is more deterministic than being
scheduled on a random thread and is more in line with CPython that only
allow signal handlers to run on the main thread.

Issue: micropython#7026
Signed-off-by: David Lechner <david@pybricks.com>
jimmo added a commit to jimmo/micropython that referenced this issue Jun 30, 2022
@jimmo
py/vm.c: Only raise pending exceptions from the main thread.
05f8c4e 
Originally we had a global `MP_STATE_VM(mp_pending_exception)`, which
meant that any thread could handle the pending exception.

In issue micropython#7026 it was decided that only the main thread should handle
pending exceptions (e.g. keyboard interrupt), and PR micropython#7026
(259d9b6 & ca920f7) changed this such that the pending exception
became per-thread, and keyboard interrupts were delivered only to the main
thread.

However, the `MP_STATE_VM(sched_state)` was still global, and this meant
that a non-main thread could race to clear `MP_STATE_VM
(sched_state)`, which meant that the main thread would never notice its
`MP_STATE_VM(mp_pending_exception)`.

This could be fixed by having mp_sched_unlock also check if the main thread
still has a pending exception, and leaving `MP_STATE_VM(sched_state)`
pending.

Instead, this commit makes `MP_STATE_VM(mp_pending_exception)` global
again, but allows only the main thread to process it. On non-threading
builds this has no effect. On threading builds, a port is able to provide
its own "am I the main thread" function, but a fallback is provided.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
jimmo added a commit to jimmo/micropython that referenced this issue Jun 30, 2022
@jimmo
py/vm.c: Only raise pending exceptions from the main thread.
9cf2e15 
Originally we had a global `MP_STATE_VM(mp_pending_exception)`, which
meant that any thread could handle the pending exception.

In issue micropython#7026 it was decided that only the main thread should handle
pending exceptions (e.g. keyboard interrupt), and PR micropython#7051
(259d9b6 & ca920f7) changed this such that the pending exception
became per-thread, and keyboard interrupts were delivered only to the main
thread.

However, the `MP_STATE_VM(sched_state)` was still global, and this meant
that a non-main thread could race to clear `MP_STATE_VM
(sched_state)`, which meant that the main thread would never notice its
`MP_STATE_VM(mp_pending_exception)`.

This could be fixed by having mp_sched_unlock also check if the main thread
still has a pending exception, and leaving `MP_STATE_VM(sched_state)`
pending.

However, there's no mechanism for pending exceptions to be delivered to
anything other than the main thread, so instead, this commit makes
`MP_STATE_VM(mp_pending_exception)` global again, but allows only the main
thread to process it. On non-threading builds this has no effect. On
threading builds, a port is able to provide its own "am I the main thread"
function, but a fallback is provided.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@jimmo jimmo mentioned this issue Jun 30, 2022
Closed
Wind-stormger pushed a commit to BPI-STEAM/micropython that referenced this issue Oct 13, 2022
@dhalbert
Merge pull request micropython#7026 from dhalbert/minor-space-savings
14c9028 
save about 112 bytes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
port-rp2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dlech @dpgeorge @kevinkk525