os/proc-kill now accepts an optional signal to send #1137

tionis · 2023-05-14T13:47:48Z

I added an optional keyword parameter to os/proc-kill that supports sending a custom signal instead of SIGKILL.
I ain't got much experience in C, so the implementation is quite simple and just converts the keywords to the relevant constants with an if-else structure and janet_cstrcmp.
Please let me know if there's a better way.
The option is ignored on Windows, as there are no signals on Windows as far as I know.

There is no test yet, as janet code can't catch signals as far as I can tell, but I tested it on my machine with a bash script:

#!/bin/env bash
abort(){
  echo bye
  exit
}

trap abort SIGINT
while true; do
  echo sleeping
  sleep 10
done

and the following janet invocation:

$ build/janet -e '(def proc (os/spawn ["./test.sh"] :p))(os/sleep 1)(os/proc-kill proc true :int)'
sleeping
bye

CosmicToast · 2023-05-14T16:43:46Z

A better approach would be to have a null-terminated struct mapping the keywords to the signals, which would make it less repetitive and easier to add new signals.
It would look about like so:

struct keyword_signal {
    const char *keyword;
    int signal;
};
static const struct keyword_signal signal_keywords[] = {
    {"abrt", SIGINT},
    {"alrm", SIGALRM},
    {"fpe", SIGFPE},
    {"hup, SIGHUP},
    {"ill", SIGILL},
    {"int", SIGINT},
    {"kill", SIGKILL},
// ... continue on like this, also consider using ifdefs to make sure all of the signals are supported on given platforms
    {NULL, 0},
};
// inside of os_proc_kill
const struct keyword_signal *ptr = signal_keywords;
while (ptr->keyword) {
    if (janet_cstrcmp(signal_kw, ptr->keyword)) {
        signal = ptr->signal;
        break;
    }
    ptr++;
}

tionis · 2023-05-14T22:04:29Z

Thanks for your Input! Your suggestion is easier to extend, but I'm not sure if that's important, as the signals are unlikely to change anytime soon.
What would ptr++ do exactly in this context? Increase the value of the pointer so that it points to a value one further down?
Your code suggestions don't seem to work for me as if (janet_cstrcmp(signal_kw, ptr->keyword)) evals as truthy for the first value (abrt) even though my test input is :int and as such it sends the wrong signal

CosmicToast · 2023-05-14T23:39:34Z

signals are unlikely to change anytime soon

A lot of your signals are actually already extensions, and as such not strictly guaranteed.
ISO C99 in section 7.14 defines only the following signals: SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM.
Your code uses several other ones.
What signals are available changes drastically depending on the operating system and operating system version (!).
As such, being able to easily add and remove (both of which need to be doable conditionally at compile time) is quite important.

Thankfully, ISO C99 also defines the signals in question to be macros (rather than enums).
It's not strictly unreasonable to expect the other ones to be macros too.
As such, you can make sure this compiles on all potential targets by also guarding every definition using an #ifdef.
If that ends up being insufficient, it's possible to also have an IANA-tzcode style macro stubs.

What would ptr++ do exactly in this context? Increase the value of the pointer so that it points to a value one further down?

Yes. In C, array access is actually pointer math - which is why C arrays are zero-indexed.
abcd[0] is strictly equivalent to abcd + 0 and so on.
The way pointer arithmetic works is that it goes up by the size of the type.
So if you have abcd + 1, if abcd is of type int32_t* then abcd + 1 is the address of abcd + 4 bytes. If abcd is of type int64_t then abcd + 1 is the address of abcd + 8 bytes. void* is not supposed to have pointer arithmetic as far as the standard goes, but basically every compiler (due to an extension originally from GNU) defines the "size" of void to be 1. offsetof is supposed to work with char*, but void* is typically used in practice.

Your code suggestions don't seem to work for me as if (janet_cstrcmp(signal_kw, ptr->keyword)) evals as truthy for the first value (abrt) even though my test input is :int and as such it sends the wrong signal

My code isn't strictly checked, sorry ^^
I'm currently packed up to move across the atlantic ocean so I'm not in a great position/situation to give exact code.
If it's still useful to do so then, I'll take a look at a more materialized patch when I have more time available.

CosmicToast · 2023-05-14T23:47:37Z

Also, I usually do a janet_keyeq against the Janet (rather than JanetKeyword) value and the C-string, and am not too familiar with the cstrcmp function (which is a likely source of the given error). Sorry about that

zevv · 2023-05-15T05:58:27Z

Given that os/... functions generally are ment to be platform agnostic, maybe handling all these specific signals will be a better fit for Spork (spork/posix/kill)?

tionis · 2023-05-15T10:56:33Z

@CosmicToast Thanks for the detailed explanation, I really appreciate it! I'll push a patch with your suggestions later.
@zevv I mainly added it to janet itself because @bakpakin suggested that sending signals could be a trivial extension of (os/proc-kill)

tionis · 2023-05-15T22:45:07Z

OK, I'm still not sure how to write a test for the feature, but the code is now cleaner, throws an error when the signal is not defined, and it's working as expected on my machine

sogaiu · 2023-05-16T01:01:42Z

suite0009.janet has a test for os/proc-kill in it that uses a spawned janet.

May be the kill signal could be specified via the optional argument to terminate a spawned janet process and then one could check in a similar manner to the existing test?

tionis · 2023-05-16T11:22:15Z

I could do that, but a test checking for other signals would be even better. But that has to wait for native signal catching support in janet I guess.

tionis · 2023-05-16T11:29:28Z

Ok, added the simple test. If someone tackles the signal catching feature at some time, a more advanced test can be added.

CosmicToast · 2023-05-16T11:44:10Z

Is there a specific reason for not having the signal definitions on windows?
Those are C99, after all.
there are some weird things about how they work on windows, and I don’t remember if kill is applicable but having the mapping around can still be useful since ISO C99 does guarantee those first couple (even on windows).

sogaiu · 2023-05-16T11:47:47Z

Are those these?

Update: seems we can link to individual pages in pdfs these days, so we can make use of CosmicToast's info about section 7.14 in the form of a link to a relevant page in a draft of ISO/IEC 9899 from 2005.

CosmicToast · 2023-05-16T13:50:06Z

Are those these?

Right.
I also mentioned them back here:

ISO C99 in section 7.14 defines only the following signals: SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM.

tionis · 2023-05-16T14:16:36Z

But so they have any effect on windows?
I thought windows has a different process management?

CosmicToast · 2023-05-16T14:47:59Z

They absolutely have an effect on Windows.
Windows uses C, and is C99 compliant - merely not POSIX compliant.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

There are absolutely weirdnesses to the Windows implementation of things.
For instance, SIGINT launches a separate thread just to handle the interrupt.
As such, cross platform signal handling is complex and error-prone.
However, all of these differences are ISO C99 compliant (they have to be!).
The C standard is just kind of awful and doesn't specify enough things to ensure behavioral interoperability, only API interoperability.

In short, there is no reason to handle Windows separately here, since all of the features are based on ISO C99.
It's up to the caller to ensure that their handling is correct on all target platforms.
And anyway, the difference in behavior primarily applies when handling signals, not so much when sending them :)
If a patch to handle signals materializes, I wouldn't have an ifdef there either, just have a compiler warning or something saying "oh you're on Windows and trying to handle SIGINT, be careful doing that".
Caller code would likely be something like (if (= (os/which) :windows) ... (signal :int myfun))

CosmicToast · 2023-05-16T15:00:54Z

src/core/os.c

@@ -624,26 +624,133 @@ JANET_CORE_FN(os_proc_wait,
 #endif
 }

+#ifndef JANET_WINDOWS


This still needs removing for windows support.

Ah just saw your comment, I'm currently trying to get a dev environment running on a windows VM to properly test the code

But thanks. I removed this ifdef now I just gotta test it all

And fix all the compile error...

sogaiu · 2023-05-16T15:59:07Z

It looks like SIGKILL is not listed as one of the 6 signals mentioned above:

SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM

On a Linux box here a janet process seems to end in response to SIGTERM (at least it did in light testing here).

Would SIGTERM be a better choice for the test perhaps?

zevv · 2023-05-16T18:30:49Z

Please notice CI failed for windows.

Afaik kill() is POSIX, and is not available on win32. C99 defines some signals but only specifies raise(), which sends a signal to the process itself.

Honestly, I still think this functionality has a better place in a POSIX-specific function in spork because os/kill can not be implemented in a portable way. Also, I think allowing a different set of signals for different platforms in the standard library will result in non-portable Janet programs.

tionis · 2023-05-16T19:29:12Z

That are some good points. Before closing this completely, I'd like to hear @bakpakin's opinion on this.

tionis · 2023-05-16T19:30:42Z

Also regarding windows: I know about the failed tests, the code doesn't compile as is on Windows, as I don't know how to send signals on Windows. The previously working code simply ignored signals on Windows.

CosmicToast · 2023-05-16T20:27:44Z

Afaik kill() is POSIX, and is not available on win32. C99 defines some signals but only specifies raise(), which sends a signal to the process itself.

That's accurate. I think keeping the code as an os/raise would make sense, though.

Honestly, I still think this functionality has a better place in a POSIX-specific function in spork because os/kill can not be implemented in a portable way.

That makes sense.

Also, I think allowing a different set of signals for different platforms in the standard library will result in non-portable Janet programs.

I don't personally think non-portable Janet programs are strictly an issue, just like non-portable Python programs are an issue, for instance.
Sometimes your code only cares about some platforms, and that's fine.
For instance, 99.9% of my code will only ever run on POSIX platforms.
The stdlib being handle anything is important (thus strict ISO C compliance), but if an end-program is non-portable, I don't really see the issue with that.

sogaiu · 2023-05-17T01:37:20Z

The stdlib being handle anything is important (thus strict ISO C compliance), but if an end-program is non-portable, I don't really see the issue with that.

I think we're likely all on the same page here.

I took the original comment to suggest there would be a tendency toward writing programs that would unintentionally be non-portable for no real good reason.

bakpakin · 2023-05-21T13:21:55Z

So as far as windows implementations, I think it is useful to look to python or other languages and their analogs. Windows does not have support for signals, so perhaps the second argument could be ignored on windows, or if provided, needs to be some default value :terminate or :kill.

tionis · 2023-05-21T13:45:33Z

I reverted to the old behaviour of ignoring signals on windows, will look at some other cross-platform languages and how they handle this

tionis · 2023-05-21T13:46:01Z

Oh and it seems that I still have to fix windows compile

sogaiu · 2023-05-21T13:49:34Z

Minor comment -- may be you can make format?

tionis · 2023-05-21T13:52:29Z

@sogaiu Sure

tionis · 2023-05-21T13:57:19Z

Pythons signal handling seems very simplistic:
https://github.com/python/cpython/blob/abb32de8c4e9541fbd0c6b14dc937193078e6955/Lib/subprocess.py#L1651

tionis · 2023-05-21T14:01:16Z

Relevant documentation in python with a mention of windows:
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal

Perhaps we could accept three signals for windows: terminate, ctrl-c-event and ctrl-break-event

sogaiu · 2023-05-21T14:50:36Z

I don't know if the info is up-to-date, but there's a comment for this SO answer that says:

SIGINT (interrupt) and non-standard SIGBREAK are implemented using a console control event handler for Ctrl+C and Ctrl+Break.

@tionis The link for "Relevant documentation in python with a mention of windows:" looks the same to me as the one for "Pythons signal handling seems very simplistic:". Is that intentional?

May be this file is relevant? Ah, may be this is the receiving end...

sogaiu · 2023-05-21T14:57:57Z

Thanks for the updated link.

tionis · 2023-05-21T14:58:03Z

Oh no, it's not, I updated it, thanks.

Regarding the SO link:
This all seems quite tedious, and I'm honestly not sure if I can implement this elegantly in any form.

To introduce some quick, working, inelegant code into core that is difficult to change later would be foolish, I think.

tionis force-pushed the master branch from d564aa7 to 9de0047 Compare May 15, 2023 22:32

os/proc-kill now accepts an optional signal to send

0b58e50

tionis force-pushed the master branch from 9de0047 to 0b58e50 Compare May 15, 2023 22:44

added simple test for signal handling in os/proc-kill using :kill

71d51c1

CosmicToast reviewed May 16, 2023

View reviewed changes

tionis force-pushed the master branch from 5cf5b9d to 71d51c1 Compare May 16, 2023 15:05

support sending signals to processes on windows

56d72ec

use SIGTERM for os/proc-kill signal test

0f35aca

CosmicToast approved these changes May 16, 2023

View reviewed changes

Merge branch 'janet-lang:master' into master

53afc2e

tionis force-pushed the master branch from 39676bd to 5aa53ee Compare May 21, 2023 13:50

ignoring signals on windows in os/proc-kill again

e53c030

tionis force-pushed the master branch from 5aa53ee to e53c030 Compare May 21, 2023 13:50

Merge branch 'janet-lang:master' into master

b4b1c7d

fixed formatting in src/core/os.c

4dfc869

bakpakin merged commit c47c2e5 into janet-lang:master May 21, 2023
7 checks passed

os/proc-kill now accepts an optional signal to send #1137

os/proc-kill now accepts an optional signal to send #1137

Conversation

tionis commented May 14, 2023

CosmicToast commented May 14, 2023 • edited

tionis commented May 14, 2023

CosmicToast commented May 14, 2023

CosmicToast commented May 14, 2023

zevv commented May 15, 2023 • edited

tionis commented May 15, 2023

tionis commented May 15, 2023

sogaiu commented May 16, 2023

tionis commented May 16, 2023

tionis commented May 16, 2023

CosmicToast commented May 16, 2023

sogaiu commented May 16, 2023 • edited

CosmicToast commented May 16, 2023

tionis commented May 16, 2023

CosmicToast commented May 16, 2023

CosmicToast May 16, 2023

Choose a reason for hiding this comment

tionis May 16, 2023

Choose a reason for hiding this comment

tionis May 16, 2023

Choose a reason for hiding this comment

tionis May 16, 2023

Choose a reason for hiding this comment

sogaiu commented May 16, 2023

zevv commented May 16, 2023 • edited

tionis commented May 16, 2023

tionis commented May 16, 2023

CosmicToast commented May 16, 2023

sogaiu commented May 17, 2023

bakpakin commented May 21, 2023

tionis commented May 21, 2023

tionis commented May 21, 2023

sogaiu commented May 21, 2023

tionis commented May 21, 2023

tionis commented May 21, 2023 • edited

tionis commented May 21, 2023 • edited

sogaiu commented May 21, 2023 • edited

sogaiu commented May 21, 2023

tionis commented May 21, 2023 • edited

CosmicToast commented May 14, 2023 •

edited

zevv commented May 15, 2023 •

edited

sogaiu commented May 16, 2023 •

edited

zevv commented May 16, 2023 •

edited

tionis commented May 21, 2023 •

edited

tionis commented May 21, 2023 •

edited

sogaiu commented May 21, 2023 •

edited

tionis commented May 21, 2023 •

edited