signal support by palaviv · Pull Request #1189 · RustPython/RustPython

palaviv · 2019-07-28T18:59:12Z

This is a very initial support in signals. I wanted to get early feedback as this is harder then expected in Rust.
This is based on the CPython signal handling. This article is actually explaining very nicely on how it works.

windelbouwman · 2019-07-29T05:08:56Z

vm/src/frame.rs

    /// Execute a single instruction.
    #[allow(clippy::cognitive_complexity)]
    fn execute_instruction(&self, vm: &VirtualMachine) -> FrameResult {
+        vm.check_signals();


I think this contraption makes sense, it inserts signals nicely at a defined point of time.

Do you think this is a good point?

I think so, we would have to wait for builtin functions to finish, but then again, we do not face threading issues.

windelbouwman · 2019-07-29T05:13:49Z

vm/src/vm.rs

+// TODO: 64
+pub const NSIG: usize = 10;
+
+pub static mut TRIGGERS: [AtomicBool; NSIG] = [


Maybe you could use Default::default() here?

Did not work. I will create a procedural macro in the future to solve this duplication.

windelbouwman · 2019-07-29T05:14:56Z

vm/src/vm.rs

+    AtomicBool::new(false),
+    AtomicBool::new(false),
+    AtomicBool::new(false),
+    AtomicBool::new(false),


Should this be a boolean flag, or a queue of events? Each event triggers a callback, so if we have multiple events happening in a short time, the boolean flag was already true, so it was not handled. I propose to create a queue of events per signal.

This will be different from the CPython implementation. Maybe we should leave this for as future improvement?

windelbouwman · 2019-07-29T05:15:31Z

vm/src/vm.rs

            trace_func,
            use_tracing: RefCell::new(false),
            settings,
+            signal_handlers: RefCell::new(HashMap::new()),


You could use Default::default() here instead of RefCell::new(HashMap::new())

windelbouwman · 2019-07-29T05:19:02Z

vm/src/vm.rs

+// TODO: 64
+pub const NSIG: usize = 10;
+
+pub static mut TRIGGERS: [AtomicBool; NSIG] = [


Having this variable as a static global would prevent multiple VirtualMachine instances. We can solve this, by moving the triggers struct into a RefCell. Place this RefCell into the signals.rs file. In the signals.rs file, create a global cell, with a trigger list. Each vm then can register a trigger list to.

Okay, my explanation is poor. Lets see how this evolves further :).

windelbouwman · 2019-07-29T05:21:12Z

vm/src/vm.rs

+                panic!("Signum bigger then NSIG");
+            }
+            let triggerd;
+            unsafe {


Please move this unsafe code into the signal module. This function should just loop over the list of triggered events. Also, you probably want to guard this for loop with some sort of lock. Another solution would be to use the std::sync::mpsc fifo, which is thread safe. This might be handy, so that the signal handlers still can add events, while this loop still is processing.

There is not problem that the an event will be added while I process the list. I will just process it in the next run.

coolreader18 · 2019-07-29T12:00:43Z

vm/src/vm.rs

+            if *signum as usize >= NSIG {
+                panic!("Signum bigger then NSIG");
+            }
+            let triggerd;


This can just be let triggerd = unsafe { ... };

palaviv · 2019-07-31T17:04:38Z

vm/src/stdlib/signal.rs

+    let sig_ign = vm.get_attribute(signal, "SIG_DFL")?;
+    let signalnum = signalnum.as_bigint().to_i32().unwrap();
+    let signal_enum = signal::Signal::from_c_int(signalnum).unwrap();
+    let sig_handler = if handler.is(&sig_dfl) {


@windelbouwman @coolreader18 any suggestion on how should I do this? This don't actually works...

Ignore this. I had a typo...

palaviv · 2019-08-03T08:26:42Z

I changed the code to use libc directly so it will work on Windows. I will test the code on Windows after adding os.kill.

windelbouwman · 2019-08-03T09:16:25Z

vm/src/frame.rs

+use crate::stdlib::signal::check_signals;
+
+#[cfg(target_arch = "wasm32")]
+fn check_signals(vm: &VirtualMachine) {}


Instead of defining a stub function for wasm, make the code on line 172 also depending on configuration.

vm/src/frame.rs

windelbouwman · 2019-08-03T09:19:17Z

vm/src/stdlib/signal.rs

+    {
+        return Err(vm.new_type_error("Hanlder must be callable".to_string()));
+    }
+    let signal = vm.import("signal", &vm.ctx.new_tuple(vec![]), 0)?;


For clarity, you might consider naming this variable signal_module.

windelbouwman · 2019-08-03T09:22:59Z

vm/src/stdlib/signal.rs

+
+#[derive(Debug)]
+enum SigMode {
+    Ign,


Does this abbreviate Ignore? We might want to use Ignore, Default and Handler here instead of the abbreviations to prevent confusion.

I removed the enum as it is not needed now that I use the same code for windows and linux.

windelbouwman · 2019-08-04T17:46:30Z

vm/src/stdlib/signal.rs

+                .get(&(signum as i32))
+                .expect("Handler should be set")
+                .clone();
+            vm.invoke(handler, vec![vm.new_int(signum), vm.get_none()])


What should happen when a signal handler throws an exception? Do you want to tackle this now, or implement it later?

I would prefer to do it later

windelbouwman reviewed Jul 29, 2019

View reviewed changes

coolreader18 approved these changes Jul 29, 2019

View reviewed changes

windelbouwman mentioned this pull request Jul 31, 2019

Implement several methods - os module #1191

Merged

palaviv commented Jul 31, 2019

View reviewed changes

palaviv force-pushed the signal branch from 518aa7b to ff64a9e Compare July 31, 2019 19:21

palaviv marked this pull request as ready for review August 2, 2019 06:44

palaviv changed the title ~~[WIP] signal support~~ signal support - UNIX Aug 2, 2019

palaviv added 17 commits August 2, 2019 09:45

Initial signal support

1b17926

Simplify signal code

5b670f8

Move static triggers array to signal.rs

f540d31

Return previous handler from signal

ca23c43

Add signal.getsignal

57cdae1

Use rust signal module directly

a7d96b7

Add signal.alarm

e8001d7

Add test for signal

c1e0799

Add signal.{SIG_IGN, SIG_DFL}

7061f1c

Iterate over triggers in check_sginals

a1af6b4

Add signal numbers

56b555b

Improve signal test

785b5d8

User arr_macro to create triggers array

61bf076

Compile nix parts only on unix

9470d75

Test signal only on unix

48da527

SIGINT not defined on windows

f3b4b28

Get SIG_IGN

7cd5e89

palaviv force-pushed the signal branch from 7f97b6e to 7cd5e89 Compare August 2, 2019 06:46

palaviv added 8 commits August 2, 2019 10:40

Fix clippy warnings

52d204c

Add empty check_signals on WASM

60b5d9d

Fix more clippy warnings

25b9f35

Add vm to WASM check_sginals

3e07d61

Use libc directly to set signal

075f2c9

Get signal numbers from libc

23cba40

Add some windows test to signal

f24c6da

Define SIG_* on windows

f600868

palaviv changed the title ~~signal support - UNIX~~ signal support Aug 3, 2019

windelbouwman reviewed Aug 3, 2019

View reviewed changes

vm/src/frame.rs Outdated Show resolved Hide resolved

windelbouwman reviewed Aug 3, 2019

View reviewed changes

palaviv added 2 commits August 3, 2019 12:26

Remove stub check_signals in WASM

c5486a6

Remove os_set_signal

5e5d46d

windelbouwman reviewed Aug 4, 2019

View reviewed changes

windelbouwman merged commit 2613ee4 into RustPython:master Aug 4, 2019

Conversation

palaviv commented Jul 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palaviv commented Aug 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windelbouwman Aug 3, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

windelbouwman Aug 3, 2019 •

edited

Loading