metacall / core

MetaCall: The ultimate polyglot programming experience.
https://metacall.io
Apache License 2.0
1.56k stars 160 forks source link

Handle kill signals in the core #121

Open akshgpt7 opened 3 years ago

akshgpt7 commented 3 years ago

Is your feature request related to a problem?

If the user presses Ctrl-C in between the execution of an async function inside metacall, the metacall_destroy() process doesn't happen, and there's an abrupt exit. This causes many deadlocks and issues, for example this.

Describe the solution you'd like

The solution should be implemented in the core itself to intercept the kill signal and call metacall_destroy() to destroy the metacall process gracefully. The signal handler can be implemented in C in the core (possibly bound to metacall_initialize() itself), so that it starts listening as soon as a metacall instance is created. For cross-platform handling, we can refer this. This can be done in a new signals module. For now, we need to intercept SIGINT and SIGTERM (and maybe SIGHUP).

First the loader will handle the Ctrl-C inside itself, then the core will destroy metacall and exit.

Describe alternatives you've considered

We can think of implementing it just at the loader level (for example, just for Node), but that will be specific to that language, and not very extensible. Implementing it in the core will make it a generalized solution and help later when we have async support for other languages too. Also, we would have to flexibility to handle more signals in future.

zikpefu commented 2 years ago

Hi I would like to formally take this issue please, thanks

thequantumquirk commented 1 year ago

Same issue is there with metacall Rust crate too

himanshuc3 commented 1 year ago

If this isn't resolved, let me dig it up @viferga

zikpefu commented 1 year ago

Hey, yeah I completely forgot about this task. Please reassign it from me please.

On Mon, Jan 2, 2023, 1:13 AM Himanshu @.***> wrote:

If this isn't resolved, let me dig it up @viferga https://urldefense.com/v3/__https://github.com/viferga__;!!PTd7Sdtyuw!T-npoSUEscK699A5EesSP5HFqOYXbKK3sR6d6sNNGBM2RC2MHw-P-Rm4Gymr7ELWvBIWRkv2Z7Fo2AryHk0yW1SmwL0$

— Reply to this email directly, view it on GitHub https://urldefense.com/v3/__https://github.com/metacall/core/issues/121*issuecomment-1368682244__;Iw!!PTd7Sdtyuw!T-npoSUEscK699A5EesSP5HFqOYXbKK3sR6d6sNNGBM2RC2MHw-P-Rm4Gymr7ELWvBIWRkv2Z7Fo2AryHk0yfRmMIRY$, or unsubscribe https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AIFCLO3MCGPXP4XW46HYOT3WQJWXLANCNFSM43IP62SA__;!!PTd7Sdtyuw!T-npoSUEscK699A5EesSP5HFqOYXbKK3sR6d6sNNGBM2RC2MHw-P-Rm4Gymr7ELWvBIWRkv2Z7Fo2AryHk0y28Pslmw$ . You are receiving this because you were assigned.Message ID: @.***>

viferga commented 1 year ago

Not resolved, but we should discuss maybe how to implement it? @zikpefu @himanshuc3

We should handle this in a cross-platform way in the Core but maybe we have to propagate the signals to node, python etc in order to notify the runtimes, so if you have a signal handler in node, python etc, it works.

Those are the main restrictions, we should think about it and how to do it, then create tests where we test the propagation of the signal into node, python etc.

viferga commented 1 year ago

We can make this in a new module called source/signal.

roysti10 commented 1 year ago

Not resolved, but we should discuss maybe how to implement it? @zikpefu @himanshuc3

We should handle this in a cross-platform way in the Core but maybe we have to propagate the signals to node, python etc in order to notify the runtimes, so if you have a signal handler in node, python etc, it works.

Those are the main restrictions, we should think about it and how to do it, then create tests where we test the propagation of the signal into node, python etc.

If we implement it in the metacall initialze itself , it would automatically propagate to the other ports right? something similar had been done with the old backtrace module as well(before it was changed to a plugin extension)

@viferga

viferga commented 1 year ago

@lucasace I am not sure 100% we should review it because maybe we need to propagate the signals to the runtimes, but indeed implementing it in the core should solve most of issues.