Skip to content

pyterm: send SIGINT to the subprocess' child#21309

Merged
OlegHahm merged 1 commit intoRIOT-OS:masterfrom
OlegHahm:pr/pyterm/sigint_at_exit
Mar 20, 2025
Merged

pyterm: send SIGINT to the subprocess' child#21309
OlegHahm merged 1 commit intoRIOT-OS:masterfrom
OlegHahm:pr/pyterm/sigint_at_exit

Conversation

@OlegHahm
Copy link
Copy Markdown
Member

@OlegHahm OlegHahm commented Mar 19, 2025

Contribution description

Upon terminating pyterm any potentially spawned process (such as RIOT native) should be terminated as well. This could be done via sending a SIGINT to these processes.

Testing procedure

Run any arbitrary RIOT application for RIOT native via make term and terminate pyterm (via /exit or ^C).

Check if all RIOT native processes have been terminated.

Issues/PRs references

Fixes #21307

@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Mar 19, 2025
@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: native Platform: This PR/issue effects the native platform labels Mar 19, 2025
@OlegHahm OlegHahm requested a review from benpicco March 19, 2025 20:14
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 20, 2025
@riot-ci
Copy link
Copy Markdown

riot-ci commented Mar 20, 2025 •
edited
Loading

Murdock results

✔️ PASSED

0de3b8a pyterm: send SIGINT to the subprocess' child

Success Failures Total Runtime
1 0 1 01m:20s

Artifacts

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Mar 20, 2025

Thank you, this fixes the issue of lingering native instances when pressing ctrl+c

They still linger around when exiting pyterm with ctrl+d though:

benpicco@carbonara ..amples/networking/gnrc/gnrc_networking % make term                                                                              
/home/benpicco/dev/RIOT/dist/tools/pyterm/pyterm -ps /home/benpicco/dev/RIOT/examples/networking/gnrc/gnrc_networking/bin/native/gnrc_networking.elf --process-args tap0 
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Welcome to pyterm!
Type '/exit' to exit.
2025-03-20 01:44:51,281 # RIOT native interrupts/signals initialized.
2025-03-20 01:44:51,281 # RIOT native board initialized.
2025-03-20 01:44:51,281 # RIOT native hardware initialization complete.
2025-03-20 01:44:51,281 # 
2025-03-20 01:44:51,282 # main(): This is RIOT! (Version: 2025.04-devel-257-g61029-dec_as_hex)
2025-03-20 01:44:51,282 # RIOT network stack example application
2025-03-20 01:44:51,282 # All up, running the shell now
> 2025-03-20 01:44:52,032 # Exiting Pyterm

benpicco@carbonara ..amples/networking/gnrc/gnrc_networking % make term
/home/benpicco/dev/RIOT/dist/tools/pyterm/pyterm -ps /home/benpicco/dev/RIOT/examples/networking/gnrc/gnrc_networking/bin/native/gnrc_networking.elf --process-args tap0 
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Welcome to pyterm!
Type '/exit' to exit.
2025-03-20 01:47:42,444 # RIOT native interrupts/signals initialized.
2025-03-20 01:47:42,445 # RIOT native board initialized.
2025-03-20 01:47:42,446 # RIOT native hardware initialization complete.
2025-03-20 01:47:42,446 # 
2025-03-20 01:47:42,448 # /home/benpicco/dev/RIOT/examples/networking/gnrc/gnrc_networking/bin/native/gnrc_networking.elf: ioctl TUNSETIFF: Device or resource busy
2025-03-20 01:47:42,450 # /home/benpicco/dev/RIOT/examples/networking/gnrc/gnrc_networking/bin/native/gnrc_networking.elf: probably the tap interface (tap0) does not exist or is already in use

benpicco
benpicco approved these changes Mar 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Works reliable for ctrl+c though

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Mar 20, 2025

They still linger around when exiting pyterm with ctrl+d though:

Strange. I never use this but there is a handler in pyterm for that. But apparently it stopped working. Will take a look into it.

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Mar 20, 2025 •
edited
Loading

Ah, no, it is still working - and for me the problem does not exist. The process is terminated on ^C and ^D. Could you check if the function in pyterm do_EOF() gets properly executed on your machine?

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Mar 20, 2025

Yes I get that

2025-03-20 14:40:59,343 # Received EOF
2025-03-20 14:40:59,343 # Exiting Pyterm

but the native process keeps lingering on

With ctrl-c I get

2025-03-20 14:42:01,152 # native: exiting
2025-03-20 14:42:01,152 # 
2025-03-20 14:42:01,153 # native: exiting
2025-03-20 14:42:01,154 # Exiting Pyterm

@OlegHahm OlegHahm changed the title pyterm: send SIGINT to the subprocess\' child pyterm: send SIGINT to the subprocess' child Mar 20, 2025
@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Mar 20, 2025

Strange. The same code is run but the behavior is different - at least for you. On my computer the output looks similar to the one you have posted but the processes are gone in any case afterwards.

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Mar 20, 2025

I tested in a OpenSuSE VM: the behavior there is that ^C terminates the processes immediately, for ^D or via /exit the processes keep lingering for some time but vanish after some seconds.

@github-actions github-actions bot removed the Platform: native Platform: This PR/issue effects the native platform label Mar 20, 2025
@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Mar 20, 2025

Can you give it another try? Should now work with ^D and /exit as well.

@OlegHahm OlegHahm added the Platform: native Platform: This PR/issue effects the native platform label Mar 20, 2025
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Mar 20, 2025 •
edited
Loading

Yes much better - now it always terminates properly 😃
Please squash!

What system are you on where it's already doing that before? I'm on Ubuntu 24.04 with Python 3.12.3

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Mar 20, 2025

Arch Linux with Python 3.13.2.

@OlegHahm OlegHahm force-pushed the pr/pyterm/sigint_at_exit branch from 5db1d92 to 0de3b8a Compare March 20, 2025 22:12
@github-actions github-actions bot removed the Platform: native Platform: This PR/issue effects the native platform label Mar 20, 2025
@OlegHahm OlegHahm enabled auto-merge March 20, 2025 22:13
@OlegHahm OlegHahm added this pull request to the merge queue Mar 20, 2025
Merged via the queue into RIOT-OS:master with commit 3312892 Mar 20, 2025
25 checks passed
@OlegHahm OlegHahm deleted the pr/pyterm/sigint_at_exit branch March 20, 2025 22:22
mguetschow
mguetschow reviewed Mar 21, 2025
View reviewed changes
dist/tools/pyterm/pyterm
import re
import codecs
import platform
import psutil
Copy link
Copy Markdown
Contributor

@mguetschow mguetschow Mar 21, 2025

Choose a reason for hiding this comment

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

@OlegHahm This introduced a new dependency that may not be installed by default. Is there an alternative to it or should we add psutil to the list of dependencies stated on https://doc.riot-os.org/getting-started.html?

Copy link
Copy Markdown
Contributor

@Enoch247 Enoch247 Apr 10, 2025

Choose a reason for hiding this comment

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

I believe this has broken pyterm for me. I now get:

josh@dragonkite:ds18$ make BOARD=stm32f429i-disc1 term
/home/josh/riot/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "115200" -ln "/tmp/pyterm-josh" -rn "2025-04-09_22.09.13-tests_ds18-stm32f429i-disc1"  
Traceback (most recent call last):
  File "/home/josh/riot/dist/tools/pyterm/pyterm", line 41, in <module>
    import psutil
ModuleNotFoundError: No module named 'psutil'
make: *** [/home/josh/riot/tests/drivers/ds18/../../../Makefile.include:873: term] Error 1

Copy link
Copy Markdown
Member Author

@OlegHahm OlegHahm Apr 10, 2025

Choose a reason for hiding this comment

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

Yes, you will need to install the Python psutil module. I didn't realize that psutil is not a standard Python module. 😔 If anyone would find the time to re-implement this feature without an external modul that would be wonderful.

Copy link
Copy Markdown
Contributor

@Enoch247 Enoch247 Apr 12, 2025

Choose a reason for hiding this comment

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

I think it is sufficient to add it to the list of pkgs to install in the getting started guide. See #21399.

@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
@Enoch247 Enoch247 mentioned this pull request Apr 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Enoch247 Enoch247 Enoch247 left review comments

@mguetschow mguetschow mguetschow left review comments

@benpicco benpicco benpicco approved these changes

Assignees

@benpicco benpicco

Labels

Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2025.04

Development

Successfully merging this pull request may close these issues.

native: Exiting pyterm does not terminate the native process any more

5 participants

@OlegHahm @riot-ci @benpicco @Enoch247 @mguetschow