Skip to content

feat(toWebRequest): add request abort signal - #1220

Open
kricsleo wants to merge 2 commits into
h3js:v1from
kricsleo:fix/web-request-signal
Open

kricsleo wants to merge 2 commits into
h3js:v1from
kricsleo:fix/web-request-signal

Conversation

@kricsleo

Copy link
Copy Markdown
Member

resolves #1204

@pi0 pi0 added the v1 label Oct 13, 2025
@pi0

pi0 commented Oct 13, 2025

Copy link
Copy Markdown
Member

Can you add a test?

@kricsleo

Copy link
Copy Markdown
Member Author

Tests added 😁

Comment thread src/utils/request.ts
abortSignal = new AbortController();

const abort = (err?: any) => abortSignal?.abort(err);
event.node.req.once("close", abort);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This listens on the request stream, so it fires when the body has been read, not when the client
disconnects. On a POST with a streaming response the signal is aborted before the handler writes
anything. It happens to work with GET because nothing reads the body.

srvx (which is what h3 v2 works on) already does this correctly in its node adapter:

  res.once("close", () => {
    if (req.errored) abort(req.errored);
    else if (!res.writableEnded) abort();
  });

Comment thread src/utils/request.ts

const abort = (err?: any) => abortSignal?.abort(err);
event.node.req.once("close", abort);
event.node.req.once("end", abort);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With proper fix (see above) this is not needed.

Comment thread test/web.test.ts
const controller = new AbortController();
const response = await client.request({
path: "/abort",
method: "GET",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should include a test for POST, too; it would surface the problems in the current implementation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants