Implement Request.signal to detect client disconnects

[npaun]

How it works:

• An AbortController is added as a member of WorkerEntrypoint.
• Its AbortSignal is passed through via ServiceWorkerGlobalScope and Request through to JS, so it can be accessed via Request.signal.
• If the client disconnects, and we've both not produced a response or thrown an exception, the AbortSignal is triggered.

[kentonv]

In #3033 I made an observation which I don't think has been covered here.

One gotcha regarding request.signal that we'll need to work out: What happens in the common case that the request is passed along to an outgoing fetch()? If the response from that fetch is immediately returned by the worker, then it makes sense that if the client disconnects, the outgoing fetch should be canceled. But what if the response of that fetch is going to be used in some other logic, maybe involving a waitUntil() that writes to cache, etc.? Then, if we cancel the subrequest based on the client disconnecting, we might break that logic.

I think we might have to say: If the signal on an outgoing fetch() is the exact same signal from the original incoming request that started the Worker, it is ignored. If that feels wrong, then we would have to introduce a compat flag to fix it without breaking anyone. But I think it's fine really... if the client disconnects and there are no waitUntil()s, then all subrequests are canceled implicitly. So this only makes a difference if there are waitUntil()s, in which case you probably don't actually want to cancel the subrequest?

[kentonv]

It looks like you force-pushed changes in response to review comments and a rebase at the same time. Unfortunately this means we reviewers are unable to see what changed since we last reviewed.

Could you please rebase this back to e5b3760 so that we can directly diff against 3afd1ad? If you need to rebase forward to fix merge conflicts, please do it as a second push, and write a comment in between, so that GitHub doesn't collapse them. (GitHub is, unfortunately, extremely annoying in this regard...)

[npaun]

@kentonv Sorry for making a mess out of the diff. I'll be more careful about this next time.
Here's a link to the diff: https://github.com/cloudflare/workerd/compare/3afd1ad..4ad11a390f1d4d1f1ee2245c71804549083e940d

[kentonv]

Still need to address the passthrough issue? (The big blockquote in my earlier comment.)

[npaun]

Yes, still to address:

• Passthrough
• ASan issue

[kentonv]

FWIW, scheduler.wait is actually available without node compat.

[npaun]

VSCode just helpfully keeps importing it. I'll get rid of it before I merge.

[npaun]

The latest push takes care of the passthrough subrequest case.

[npaun]

I'm treating req.clone() and new Request(req) differently in that former always clones the associated signal, while the latter will respect ignoreForSubrequests and ignore the signal of the incoming fetch. Let me know if that doesn't sound right.

[kentonv]

Hmm, this is not the solution I was expecting. I thought that the implementation of fetch() would be modified to ignore the signal if it came from the original top-level request. Why should cloning the request (whether via .clone() or the constructor) drop the signal?

[kentonv]

Actually if someone just does fetch(originalRequest), the request is not cloned at all, so doesn't this not even solve that case?

[npaun]

It does work because fetch calls the Request constructor again, but yeah this isn't the right way to accomplish this. I fixed it.