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

Feature proposal: Signal client disconnect using Request.signal #3033

Open
brettwillis opened this issue Oct 30, 2024 · 4 comments
Open

Feature proposal: Signal client disconnect using Request.signal #3033

brettwillis opened this issue Oct 30, 2024 · 4 comments
Labels
feature request Request for Workers team to add a feature

Comments

@brettwillis
Copy link

As I understand it, when a client disconnects, the context/"thread" for a request is aborted as is. And if ctx.waitUntil() was called, then those promises are allowed some time to resolve.

When you are running async callbacks using ctx.waitUntil(promise), there is no easy way to know that the client has disconnected.

Conceptually, if you responded with a ReadableStream, then controller.enqueue(chunk) should throw or writer.write(chunk) of the writable side should reject on the next write. This would require keeping the context alive with ctx.waitUntil(writable.closed) or something like that, otherwise the context would just die immediately anyway. However I've found that these don't seem to reliably throw/reject when the client disconnects. Perhaps that is a separate issue.

Much of what I'm describing above is from experience/testing or examining workerd rather than documentation, so feel free to correct me.

Proposal

The Request.signal API is an existing standard API which reflects the AbortSignal passed when constructing the request. However the signal property of the incoming request currently does nothing.

I propose that the Workers runtime utilise this existing API by "aborting" the signal of the incoming request when a client disconnects. Any scripts that are interested in reacting when the client has disconnected can utilise this signal.

If an abort listener has been registered using request.signal.addEventListener('abort', ...) then the runtime will call those handler(s) when the client disconnects, before aborting the context. If the handler(s) synchronously call ctx.waitUntil(promise) then that will keep the context alive for a further period of time.

Credit to uri2 here for this inspiration of this idea, as other proposals I had entertained were new non-standard APIs such as ctx.addEventListener('end', ...) etc.

I'm happy to contribute if this proposal makes it through.

Use cases

The use-cases I require this for are scenarios when the Worker is generating a long-running response stream on the fly, for example Server-sent events.

When the client disconnects from the stream, it is useful to react to perform logging, or do cleanup such as signalling user presence in a database.

Currently the request will either die immediately, or if ctx.waitUntil(writable.closed) was called then it will continue running unknowingly until it gets terminated anyway 30s or so later.

@jasnell jasnell added the feature request Request for Workers team to add a feature label Nov 3, 2024
@kentonv
Copy link
Member

kentonv commented Nov 11, 2024

FWIW, I agree this is the right API.

(But I don't currently know when we'll be able to implement it.)

@brettwillis
Copy link
Author

(But I don't currently know when we'll be able to implement it.)

I'd be open to contributing this. But I wouldn't want to get started until the proposal has been through the appropriate reviews etc? Let me know.

@ricardopolo
Copy link

👍 to this feature, cloudflare should support abort requests

in the meantime, anyone have any workaround in mind?

@kentonv
Copy link
Member

kentonv commented Nov 12, 2024

@mikenomitch @irvinebroque thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature
Projects
None yet
Development

No branches or pull requests

4 participants