Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Request.signal to detect client disconnects #3488

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

npaun
Copy link
Member

@npaun npaun commented Feb 6, 2025

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.

@npaun npaun requested review from jasnell and kentonv February 6, 2025 23:43
@npaun npaun requested review from a team as code owners February 6, 2025 23:43
@npaun npaun linked an issue Feb 6, 2025 that may be closed by this pull request
@kentonv
Copy link
Member

kentonv commented Feb 7, 2025

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?

@npaun npaun force-pushed the npaun/request-signal branch from 3afd1ad to 9088343 Compare February 7, 2025 22:41
@kentonv
Copy link
Member

kentonv commented Feb 10, 2025

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 npaun force-pushed the npaun/request-signal branch 3 times, most recently from ae43332 to 4ad11a3 Compare February 10, 2025 15:49
@npaun
Copy link
Member Author

npaun commented Feb 10, 2025

@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

@npaun npaun force-pushed the npaun/request-signal branch from 4ad11a3 to d43950e Compare February 10, 2025 16:12
@kentonv
Copy link
Member

kentonv commented Feb 10, 2025

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

@npaun
Copy link
Member Author

npaun commented Feb 10, 2025

Yes, still to address:

  • Passthrough
  • ASan issue

@npaun npaun force-pushed the npaun/request-signal branch 2 times, most recently from d977e53 to 3651ba7 Compare February 12, 2025 16:52
@npaun
Copy link
Member Author

npaun commented Feb 12, 2025

The latest push takes care of the passthrough subrequest case.

@npaun
Copy link
Member Author

npaun commented Feb 28, 2025

...

@npaun npaun requested review from jasnell and kentonv February 28, 2025 20:31
@npaun npaun force-pushed the npaun/request-signal branch 2 times, most recently from 7697dde to eeabf8f Compare February 28, 2025 22:56
@npaun npaun force-pushed the npaun/request-signal branch from eeabf8f to a4ad318 Compare March 2, 2025 22:11
@npaun npaun requested a review from jasnell March 2, 2025 22:11
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM but let's also have @kentonv review. Given the complexity of the change it's easy to miss something in this

@npaun npaun force-pushed the npaun/request-signal branch from a4ad318 to de5fddb Compare March 5, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature proposal: Signal client disconnect using Request.signal
3 participants