solid-start icon indicating copy to clipboard operation
solid-start copied to clipboard

[Feat]: Better DX for Abort events

Open tri2820 opened this issue 11 months ago • 3 comments

Duplicates

  • [x] I have searched the existing issues

Latest version

  • [x] I have tested the latest version

Current behavior 😯

When the request is aborted, the abort event is not fired within server API route.

Example server API route

import type { APIEvent } from "@solidjs/start/server";
export async function GET(event: APIEvent) {

    event.request.signal.addEventListener("abort", () => {
        console.log('abort detected')
    })

    let i = 0;
    while (i < 20) {
        console.log('looping', i++, event.request.signal.aborted)
        await new Promise(resolve => setTimeout(resolve, 1000))
    }

    return new Response(null, { status: 200 })
}

Example browser triggers

<button
onClick={() => {
  controller = new AbortController();
  fetch("/api/test", {
    method: "GET",
    signal: new AbortController().signal,
  })
    .then((response) => {
      console.log("response", response);
    })
    .catch((error) => {
      console.log("error", error);
    });
}}
>
Call
</button>
<button
onClick={() => {
  console.log("controller", controller);
  controller?.abort("I am aborting this request");
  controller = undefined;
}}
>
Abort
</button>

Expected behavior 🤔

When user cancel the request in browser, the abort event should be fired.

Steps to reproduce 🕹

Steps:

  1. Create an API route
  2. Add abort listener inside the API route
  3. In the browser, fetch & abort
  4. Notice that the abort event is not fired inside the API route

Context 🔦

I'm building an AI chat app that streams the token from the AI provider back to the user. Currently when the user clicks aborting the request, the api route could not detect that and keep streaming from the AI provider, wasting token.

Not sure if related but Next.js used to have a similar issue https://github.com/vercel/next.js/discussions/48682

Your environment 🌎

Linux denpa 6.12.10-arch1-1 #1 SMP PREEMPT_DYNAMIC Sat, 18 Jan 2025 02:26:57 +0000 x86_64 GNU/Linux

bun --version
1.2.0

node --version
v23.4.0

"dependencies": {
    "@solidjs/meta": "^0.29.4",
    "@solidjs/router": "^0.15.0",
    "@solidjs/start": "^1.0.11",
    "solid-js": "^1.9.2",
    "vinxi": "^0.4.3"
  },

tri2820 avatar Feb 03 '25 14:02 tri2820

Update: After some digging I found out that we can listen to the close event on nativeEvent for this.

import { getRequestEvent } from "solid-js/web";

export async function GET() {
    const event = getRequestEvent()!;
    event.nativeEvent.node.req.addListener('close', () => {
        console.log('Client closed connection');
    })

    await new Promise(resolve => setTimeout(resolve, 5000));

    return new Response(`I'm alive!`, {
        status: 200,
    });
}

Nevertheless it would be good if event.request.signal.addEventListener("abort", ...) is supported

tri2820 avatar Feb 06 '25 23:02 tri2820

Thanks for this issue and the thorough explanation, @tri2820 Imho, that's definitely something that's worth having a better DX around (ping @katywings for her 2cents).


If you have time, it would be great if you could add a reproduction to our /tests workspace.

You can start the PR with the failing test and then we can have a clear path moving forward.

Meanwhile, this workaround is worthy of the docs, so I'm pinging @LadyBluenotes in case she wants to create an issue there too :)

As soon as I can get my hands on trying the workaround, I can help writing the guide (but you're also ofc welcomed to do so if you want)

atilafassina avatar Feb 12 '25 08:02 atilafassina

@tri2820 Thank you for bringing this up. I also think that having a better DX around abort sounds like a sensible addition 💯.

The first question coming to my mind is if event.nativeEvent.node.req.addListener('close', ...) is the proper way to do this from a nitro/h3 perspective. Maybe @pi0 can give us some insight about this 🙂. Touching event.node directly is usually not recommended by h3, so I wonder if there is some "more official" way to do this, or if it already is planned for a future update?

A similar question was asked in the Nitro Discussions under nitrojs/nitro#1541 but is unanswered 🙈.

katywings avatar Feb 12 '25 13:02 katywings