Proposal: Callback based Signal API

[kt3k]

This proposal suggests removing the existing Signal APIs and instead suggests adding Callback based Signal APIs.

Suggested APIs

const handler = () => { console.log("Got SIGINT"); }
Deno.addSignalListener("SIGINT", handler); // attach to the SIGINT event
Deno.removeSignalListener("SIGINT", handler); // detach from the SIGINT event

Note: This proposal originally suggested globalThis.addEventListener("SIGINT", ...) globalThis.removeEventListener("SIGINT", ...) APIs, but I changed them to the above because of the concern about the side effect of attaching listeners. refs: #11158 (comment) #11158 (comment)

Motivations

Currently we have AsyncIterable based API in the unstable namespace.

for await (const _ of Deno.signal(Deno.Signal.SIGINT)) {
  console.log("Got SIGINT");
}

And optionally Promise based API.

await Deno.signal(Deno.Signal.SIGINT); // Resolves when the first signal comes
console.log("Got SIGINT");

These APIs are confusing to the users because of several reasons.

(1.) Because signal handlers use AsyncUnref Op internally, the runtime can exit in the middle of for-await loop without breaking it, and this is confusing.

async function listenSigint() {
  for await (const _ of Deno.signal(Deno.Signal.SIGINT)) {
    console.log("Got SIGINT");
  }
  console.log("Finished the loop")
}
listenSigint();

The above example exits immediately without printing anything.

(2.) Similarly to (1.), the runtime can exit even when the promise is still in pending state. This behavior is confusing (There is no other promise APIs which works like this)

async function listenSigint() {
  await Deno.signal(Deno.Signal.SIGINT)
  console.log("Got SIGINT");
}
listenSigint();

The above example exits immediately without printing anything.

(3.) One of the reasons to choose async iterable API design is that it can handle the back pressure effectively, but in the case of signals, it isn't used in such a way (A program isn't flooded by signals in general). So this reasoning doesn't apply to signal APIs.

Callback based API addresses these problems in a natural way

Deno.addSignalListener("SIGINT", () => {
  console.log("Got SIGINT");
});

The above program exits immediately, but I think this doesn't cause any surprise to the users unlike async iterable based API.

[caspervonb]

At the system call level, signals are exposed as interrupts so handling them as events makes so much more sense!
It would even make it much simpler to add default behaviours because those could just be negated with preventDefault().

[ry]

Rather than using addEventListener we could do something like

Deno.onSignal("SIGINT", () => {
  console.log("Got SIGINT")
});

[piscisaureus]

Rather than using addEventListener we could do something like Deno.onSignal

But why do you think that is better than addEventListener?

[bartlomieju]

I generally like the proposal for addEventListener - what worries me though is that we'll have to complicate the logic for event listeners just to start/stop polling for signals (ie. dispatch proper ops) by matching event name prefix to SIG. Maybe we could model it after event API but do something like Deno.addSignalListener() and Deno.removeSignalListener()? Then the two concepts wouldn't cross each other.

[caspervonb]

I generally like the proposal for addEventListener - what worries me though is that we'll have to complicate the logic for event listeners just to start/stop polling for signals (ie. dispatch proper ops) by matching event name prefix to SIG. Maybe we could model it after event API but do something like Deno.addSignalListener() and Deno.removeSignalListener()? Then the two concepts wouldn't cross each other.

Presumably we would always have our own signal event handlers and dispatch the events rather than lazy initialisation? We need to do that in order to support things like spin up inspector from SIGUSR1 (which would be prevented with preventDefault()`).

[kt3k]

@ry

Rather than using addEventListener we could do something like

Deno.onSignal("SIGINT", () => {
  console.log("Got SIGINT")
});

My concern about this design is that how to unbind from the signal is not obvious. I can think of 3 different designs for unbinding, for example, and it seems difficult to decide which is better than the other.

Deno.onSignal("SIGINT", handler)
Deno.unbindSignal("SIGINT", handler); // inspired by add/removeEventListener

const handleId = Deno.onSignal("SIGNAL", handler);
Deno.clearSignal(handleId); // inspired by clearTimeout

const cancel = Deno.onSignal("SIGNAL", handler);
cancel();

[kt3k]

@bartlomieju I'm open to Deno.addSignalListener design. It solves the problem in a similar way and how to unbind is almost obvious from the similarity of the name to addEventListener.

But personally I don't think mixing events to addEventListener adds that much complexity to the implementation. I'm thinking of implementing like the below.

window.addEventListener = function addEventListener(type, listener, options) {
  if (isSignalType(type)) {
    addSignalHandler(type, listener, options);
  } else {
    EventTarget.prototype.addEventListner.call(globalThis, type, listener, options);
  }
}

With the above construction, signal related stuff can be separated into isSignalType and addSignalHandler, and they can be implemented independently from the main event logic.

[lucacasonato]

@kt3k That implementation would likely break some WPT once I fix up EventTarget. Is there precedent for "conditional dispatch" on the web platform?

[kt3k]

@lucacasonato Could you elaborate on that? What do you mean by conditional dispatch?

[lucacasonato]

I.e. an event is only dispatched if there is a listener. I vaguely remember the DOM spec saying that dispatchers should be unaware of the number of event listeners.

Another issue is modifying the addEventListener method on Window. That method is actually inherited from EventTarget, so adding that method to an extended class would break if a user does EventTarget.prototype.addEventListener.call(window, "SIGINT", (e) => console.log(e)).

[bartlomieju]

In the light of Luca's argument I'm even more in favor of Deno.addSignalListener() and Deno.removeSignalListener()

[kt3k]

@lucacasonato

I.e. an event is only dispatched if there is a listener. I vaguely remember the DOM spec saying that dispatchers should be unaware of the number of event listeners.

I don't know whether dom spec has such rules, but I doubt dom implementations dispatch mousemove or scroll events all the time when the users interacting with pages. That would be a significant performance penalty.

so adding that method to an extended class would break if a user does EventTarget.prototype.addEventListener.call(window, "SIGINT", (e) => console.log(e)).

Do we need to support EventTarget.prototype.addEventListener.call(window, "SIGINT", (e) => console.log(e))? If this is absolutely necessary, we can add some internally hook in EventTarget.prototype.addEventListener implementation

EventTarget side

addEventLister(type, callback, options) {
  ...
  const target = this ?? globalThis;
  target[Symbol.for("Deno.addEventListenerHook")]?.(type, callback, options);
}

window side

window[Symbol.for("Deno.addEventListenerHook")] = function addSignalHandler(type, callback, options) { ... }

[andreubotella]

I don't know whether dom spec has such rules, but I doubt dom implementations dispatch mousemove or scroll events all the time when the users interacting with pages. That would be a significant performance penalty.

I believe the section @lucacasonato is talking about is https://dom.spec.whatwg.org/#observing-event-listeners. It doesn't require that the presence or absence of listeners not be observable to the code dispatching the event, but it does say that adding an empty listener mustn't change the event's behavior, and it should only have an effect on performance if there's no reasonable alternative. And in this case, adding an empty listener would change the behavior, since it would prevent the default signal handler from running.

But there is a way to make this work with the behavior required for DOM events: by requiring the listeners to call event.preventDefault() to prevent the default signal handler from running. That way there would be no observable difference between having no listeners registered and having a empty one.

[kt3k]

@andreubotella Thank you for the input.

But there is a way to make this work with the behavior required for DOM events: by requiring the listeners to call event.preventDefault() to prevent the default signal handler from running. That way there would be no observable difference between having no listeners registered and having a empty one.

This suggestion makes sense to me, but this sounds difficult to implement to me. We even can't restore the default behavior after unbinding from the signal at the moment (#7164).

Now I'd prefer Deno.addSignalListener design as we don't need to care about side effect problem with it.

[kt3k]

I updated the main description of the issue from globalThis.addEventListener design to Deno.addSignalListner design.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.