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

[tionis]

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]

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]

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]

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]

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]

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]

@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]

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]

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]

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]

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

[CosmicToast]

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]

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]

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]

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

[CosmicToast]

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))

[sogaiu]

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]

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]

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

[tionis]

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]

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]

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]

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]

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]

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

[sogaiu]

Minor comment -- may be you can make format?

[tionis]

@sogaiu Sure

[tionis]

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

[tionis]

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]

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]

Thanks for the updated link.

[tionis]

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.