effection icon indicating copy to clipboard operation
effection copied to clipboard

Don't register SIGTERM listener on Windows

Open proctord-flal opened this issue 8 months ago • 4 comments

Windows does not support SIGTERM.

References:

  • https://docs.deno.com/api/deno/~/Deno.addSignalListener
  • https://docs.deno.com/api/deno/~/Deno.build.os
  • https://nodejs.org/api/process.html#processplatform
  • https://learn.microsoft.com/en-us/previous-versions/xdkz3x12(v=vs.140)

Motivation

I'd like to use Effection on Windows/Deno.

Approach

Currently, Effection is unusable on Deno on Windows because Deno.addSignalListener("SIGTERM", ...) throws, because SIGTERM is not supported by Windows.

To solve this problem, I guard signal registration with a platform check.

proctord-flal avatar Apr 18 '25 03:04 proctord-flal

@proctord-flal Is this a problem on both Deno and Node, or just Deno?

cowboyd avatar May 03 '25 19:05 cowboyd

@cowboyd

Not a problem on Windows/Node, only on Windows/Deno.

node --version
v23.11.0
deno --version
deno 2.2.11 (stable, release, x86_64-pc-windows-msvc)
v8 13.5.212.10-rusty
typescript 5.7.3
systeminfo
OS Name:                       Microsoft Windows 11 Pro
OS Version:                    10.0.26100 N/A Build 26100
main.mjs
import { main } from 'effection';

await main(function* () {
    console.log("hello world");
});
package.json
{
  "dependencies": {
    "effection": "^3.5.0"
  }
}
node main.mjs
hello world
deno main.mjs
$ deno --allow-all main.mjs
TypeError: Windows only supports ctrl-c (SIGINT) and ctrl-break (SIGBREAK), but got SIGTERM
    at bindSignal (ext:deno_os/40_signals.js:14:10)
    at Object.addSignalListener (ext:deno_os/40_signals.js:54:19)
    at Object.deno (file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/main.js:76:30)
    at deno.next (<anonymous>)
    at withHost (file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/main.js:133:26)
    at withHost.next (<anonymous>)
    at file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/main.js:71:20
    at Generator.next (<anonymous>)
    at file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/instructions.js:65:21
    at Generator.next (<anonymous>)

proctord-flal avatar May 03 '25 20:05 proctord-flal

@proctord-flal Thanks for the clarification!

Given that, do we need to remove the SIGTERM handler for node?

cowboyd avatar May 03 '25 22:05 cowboyd

Also, we should probably run the tests on windows.

cowboyd avatar May 03 '25 22:05 cowboyd

This has been fixed by #1031 Thanks @proctord-flal! Even though we didn't end up going with this PR since most of the work was getting our CI setup to run on Windows, it did put it on our radar.

cowboyd avatar Oct 07 '25 16:10 cowboyd

🤦 I should have used this as my starting point. Sorry I missed it.

taras avatar Oct 07 '25 16:10 taras