Correct exitcode when running through `subprocess.run` #4454

moreal · 2023-01-15T15:29:02Z

This pull request fixes exitcode of SIGINT (i.e., KeyboardInterrupt) when running through subprocess.run.

You can run the below script.

import sys, subprocess; subprocess.run([sys.executable, "-c", "raise KeyboardInterrupt"])

CPython

Python 3.10.9 (main, Dec 15 2022, 17:11:09) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys, subprocess; subprocess.run([sys.executable, "-c", "raise KeyboardInterrupt"])
Traceback (most recent call last):
  File "<string>", line 1, in <module>
KeyboardInterrupt
CompletedProcess(args=['/opt/homebrew/opt/python@3.10/bin/python3.10', '-c', 'raise KeyboardInterrupt'], returncode=-2)

Before (RustPython main)

>>>>> import sys, subprocess; subprocess.run([sys.executable, "-c", "raise KeyboardInterrupt"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyboardInterrupt
CompletedProcess(args=['/path/to/RustPython/RustPython/target/debug/rustpython', '-c', 'raise KeyboardInterrupt'], returncode=130)

After (RustPython)

Welcome to the magnificent Rust Python 0.2.0 interpreter 😱 🖖
>>>>> import sys, subprocess; subprocess.run([sys.executable, "-c", "raise KeyboardInterrupt"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyboardInterrupt
CompletedProcess(args=['/path/to/RustPython/RustPython/target/debug/rustpython', '-c', 'raise KeyboardInterrupt'], returncode=-2)

Related links

moreal · 2023-01-18T05:24:05Z

I researched about failing in test_subprocess and it was because it kills the process before flushing sys.stderr and sys.stdout. I fixed it by calling interpreter::flush_std before killing process.

fanninpm

LGTM

fanninpm · 2023-01-18T12:37:11Z

Lib/test/test_runpy.py

+    # TODO: RUSTPYTHON
+    if sys.platform == "win32":
+        windows_only_failed_tests = [
+            test_pymain_run_file,
+            test_pymain_run_file_runpy_run_module,
+            test_pymain_run_file_runpy_run_module_as_main,
+            test_pymain_run_command_run_module,
+            test_pymain_run_command,
+            test_pymain_run_module
+        ]
+        for t in windows_only_failed_tests:
+            unittest.expectedFailure(t)
+


fanninpm · 2023-01-18T12:45:24Z

vm/src/vm/mod.rs

+                    unsafe {
+                        if sigaction(
+                            SIGINT,
+                            &SigAction::new(
+                                nix::sys::signal::SigHandler::SigDfl,
+                                SaFlags::SA_ONSTACK,
+                                SigSet::empty(),
+                            ),
+                        )
+                        .is_ok()
+                        {
+                            interpreter::flush_std(self);
+                            kill(getpid(), SIGINT).expect("Expect to be killed.");
+                        }
+                    }
+


This seems like a lot of unsafe.

Does Rust allow, e.g.

if (unsafe { dangerous() }) { // ... }

…or is the syntax too restrictive for that?

I didn't know the syntax is available. Only sigaction function needs unsafe. So I'll apply it like with cargo fmt.

Suggested change

unsafe {

if sigaction(

SIGINT,

&SigAction::new(

nix::sys::signal::SigHandler::SigDfl,

SaFlags::SA_ONSTACK,

SigSet::empty(),

),

)

.is_ok()

{

interpreter::flush_std(self);

kill(getpid(), SIGINT).expect("Expect to be killed.");

}

}

if unsafe { sigaction(

SIGINT,

&SigAction::new(

nix::sys::signal::SigHandler::SigDfl,

SaFlags::SA_ONSTACK,

SigSet::empty(),

),

) }

.is_ok()

{

interpreter::flush_std(self);

kill(getpid(), SIGINT).expect("Expect to be killed.");

}

You can see the change in f4a3049 commit.

in general, you may want to make a new variable for complex expressions.

let action = SigAction::new( nix::sys::signal::SigHandler::SigDfl, SaFlags::SA_ONSTACK, SigSet::empty(), ); let r = unsafe { sigaction( SIGINT, &action, ) }; if r.is_ok() { ... }

I applied it in a3507e7 commit 🙏🏻

p.s. I'm not sure why review request was removed 🤔

youknowone · 2023-01-21T02:18:07Z

Thank you!

moreal marked this pull request as draft January 15, 2023 15:41

moreal force-pushed the sigaction branch from 01b4b69 to 22a4db5 Compare January 15, 2023 15:52

moreal marked this pull request as ready for review January 15, 2023 15:53

youknowone requested a review from coolreader18 January 15, 2023 16:12

moreal force-pushed the sigaction branch from 22a4db5 to 13f6f4e Compare January 15, 2023 21:47

Correct exitcode when running through subprocess.run

f0c427b

moreal force-pushed the sigaction branch from 13f6f4e to f0c427b Compare January 18, 2023 05:09

Reuse flush_std

52afc1a

moreal marked this pull request as draft January 18, 2023 06:52

moreal marked this pull request as ready for review January 18, 2023 06:59

moreal force-pushed the sigaction branch from 992dc08 to a88e537 Compare January 18, 2023 08:18

Unmark resolved tests

189a490

moreal force-pushed the sigaction branch from a88e537 to 189a490 Compare January 18, 2023 10:42

fanninpm approved these changes Jan 18, 2023

View reviewed changes

Use unsafe only where necessary

a3507e7

moreal force-pushed the sigaction branch from f4a3049 to a3507e7 Compare January 18, 2023 21:55

moreal requested review from youknowone and removed request for coolreader18 January 18, 2023 22:00

youknowone merged commit 225efca into RustPython:main Jan 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correct exitcode when running through `subprocess.run` #4454

Correct exitcode when running through `subprocess.run` #4454

Uh oh!

moreal commented Jan 15, 2023 •

edited

Loading

Uh oh!

moreal commented Jan 18, 2023 •

edited

Loading

Uh oh!

fanninpm left a comment

Uh oh!

fanninpm Jan 18, 2023

Uh oh!

fanninpm Jan 18, 2023

Uh oh!

moreal Jan 18, 2023

Uh oh!

moreal Jan 18, 2023

Uh oh!

youknowone Jan 18, 2023 •

edited

Loading

Uh oh!

moreal Jan 18, 2023 •

edited

Loading

Uh oh!

youknowone commented Jan 21, 2023

Uh oh!

Uh oh!

Correct exitcode when running through subprocess.run #4454

Correct exitcode when running through subprocess.run #4454

Uh oh!

Conversation

moreal commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CPython

Before (RustPython main)

After (RustPython)

Related links

Uh oh!

moreal commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanninpm left a comment

Choose a reason for hiding this comment

Uh oh!

fanninpm Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

fanninpm Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

moreal Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

moreal Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moreal Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Jan 21, 2023

Uh oh!

Uh oh!

Correct exitcode when running through `subprocess.run` #4454

Correct exitcode when running through `subprocess.run` #4454

moreal commented Jan 15, 2023 •

edited

Loading

moreal commented Jan 18, 2023 •

edited

Loading

youknowone Jan 18, 2023 •

edited

Loading

moreal Jan 18, 2023 •

edited

Loading