feat(core) support AbortSignal in Deno.listen

[benjamingr]

Refs #10181

This continues the work in #10943 and #11568 adds AbortSignal support to Deno.listen (and since it uses the same infrastructure writeTextFile)

There shouldn't be a DOMException here for AbortError looking at the guidance at WHATWG (the guidance is about the signature and errors of methods returning promises which isn't really the case here). I opted to do the same thing we did for Node.js

In all likelihood there should be weak event listeners (internal) in Deno core but that's a bigger refactor and honestly the event_target implementation is kind of a mess.

[evanwashere]

Suggested change

	if (options?.signal) {
	// TODO(benjamingr) there is memory leak potential here if the signal outlives the
	// server significantly it can retain it. Ideally Deno would have internal weak
	// event listener like Node does *or* it would have an event for the server closing.
	options?.signal?.addEventListener("abort", () => listener.close(), {
	once: true,
	});
	}
	if (options?.signal) {
	const weak = new WeakRef(listener);
	options?.signal.addEventListener("abort", () => weak.deref()?.close(), {
	once: true,
	});
	}

[benjamingr]

That won't help - it still leaks the listener :) Happy to elaborate if that is unclear

[evanwashere]

it's true that function captures everything
if v8 is failing to optimize that without any invocations, we can move event callback creation outside scope

[benjamingr]

@evanwashere even if the capture was only of the listener this is still a memory leak since the listener itself is captured.

Weak event listeners need to be baked into the event target implementation itself for this to not leak (here is an example how I did it in Node.js nodejs/node#36607 and I am happy to point to whatwg discussions).

That said - another (not bad) alternative is to expose a way to know when the listener is closed (and remove the handler then) but I didn't want to propose a change of the API of Deno.listen just for that :]

(This leak is small and not very problematic to be fair)

[evanwashere]

listener is wrapped in weakref, so capture won't matter and problem will be fixed

{
  const listener = {...};
  if (signal) signal.add('event', createListenerAbortCallback(new WeakRef(listener)));
}

function createListenerAbortCallback(weakref) {
  return () => weakref.deref()?.close();
}

[benjamingr]

yes, the fact it needs a separate fix is why I added a TODO for it :) The actual leak here is contrived, edge-casey and not that bad.

[bnoordhuis]

Bit of a drive-by comment but can't this be fixed with a WeakMap?

const weakrefs = new WeakMap();

function listen({ hostname, ...options }) {
  // ...
  if (signal) {
    weakrefs.set(signal, new WeakRef(listener));
    signal.addEventListener("abort", handleAbort);
  }
  // ...
}

function handleAbort(e) {
  weakrefs.get(e.target)?.deref()?.close();
}

(Use of primordials omitted for brevity.)

[benjamingr]

@bnoordhuis that would still leak the handleAbort function reference.

[bnoordhuis]

That's not a problem though, is it? The solutions outlined earlier leak because they create new closures every time, whereas the handleAbort() closure is created only once (and at the top-level so it's going to stay around forever anyway.)

[benjamingr]

@bnoordhuis is the handleAbort reference still going to be in the listeners array?

The "interesting" case here is a long-lived signal (imagine a signal owned by a controller that gets aborted on SIGINT for graceful shutdown) that gets registered often (imagine in this case you start a server on every request and do something).

Since this is Deno.listen it's not very likely - but that's the case we guarded against in Node

[CLAassistant]

All committers have signed the CLA.

[stale]

This pull request 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.