Skip to content

http: add req.signal to IncomingMessage#62541

Open
akshatsrivastava11 wants to merge 4 commits intonodejs:mainfrom
akshatsrivastava11:akshatsrivastava11/feature/req.signal
Open

http: add req.signal to IncomingMessage#62541
akshatsrivastava11 wants to merge 4 commits intonodejs:mainfrom
akshatsrivastava11:akshatsrivastava11/feature/req.signal

Conversation

@akshatsrivastava11
Copy link
Copy Markdown

Fixes: #62481

Summary

Adds a lazy signal getter to IncomingMessage that returns an
AbortSignal which aborts when the request is closed or aborted.
This mirrors the Web Fetch API's Request.signal and Deno's
request.signal.

Motivation

Currently, cancelling async work (DB queries, fetch calls) when a
client disconnects requires manual boilerplate in every request handler:

// before
server.on('request', async (req, res) => {
  const ac = new AbortController();
  req.on('close', () => ac.abort());
  const data = await fetch('https://slow-api.com', { signal: ac.signal });
  res.end(JSON.stringify(data));
});

With this change:

// after
server.on('request', async (req, res) => {
  const data = await fetch('https://slow-api.com', { signal: req.signal });
  res.end(JSON.stringify(data));
});

Design

Lazy initializationAbortController is only created when
req.signal is first accessed. Zero overhead for handlers that
do not use it.

Single listener — listens on this.once('close') and
this.once('abort') rather than socket.once('close') directly,
since the request stream's close event fires in all teardown paths:

  • socket close propagates through _destroy() to req close
  • socketless req.destroy() fires req close directly
  • client abort fires req abort directly

Race condition — if .signal is accessed after req.destroy()
has already fired, this.destroyed is checked and the signal is
aborted immediately rather than registering a listener that would
never fire.

configurable: true — allows frameworks (Express, Fastify, Koa)
to override the property on their own subclasses.

Changes

  • lib/_http_incoming.js — adds signal getter to IncomingMessage
  • test/parallel/test-http-request-signal.js — adds tests

Tests

  • req.signal is an AbortSignal and not aborted initially
  • signal aborts when 'abort' event fires
  • signal aborts when 'close' event fires (client disconnect)
  • signal is pre-aborted if accessed after req.destroy() (race condition)
  • multiple accesses return the same signal instance (lazy init)

Fixes: #62481

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Apr 1, 2026
@akshatsrivastava11
Copy link
Copy Markdown
Author

@mcollina
could you please let me know if this is along the lines of what you had in mind, or if I should change anything ?

Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you add a test how this would be used in http.request() response?

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.71%. Comparing base (0dfdec9) to head (bdf63e1).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62541      +/-   ##
==========================================
- Coverage   89.71%   89.71%   -0.01%     
==========================================
  Files         695      695              
  Lines      214154   214177      +23     
  Branches    41009    41014       +5     
==========================================
+ Hits       192132   192143      +11     
- Misses      14075    14078       +3     
- Partials     7947     7956       +9     
Files with missing lines Coverage Δ
lib/_http_incoming.js 99.38% <100.00%> (+0.03%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@akshatsrivastava11
Copy link
Copy Markdown
Author

@mcollina I've added a test for the http.request() response case in the latest commit. It creates a server that sends partial data and keeps the connection open, then destroys the client request mid-response and verifies that res.signal aborts. Let me know if that's what you had in mind or if anything needs adjusting.

Copy link
Copy Markdown

@slagiewka slagiewka left a comment

Choose a reason for hiding this comment

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

Great to see it happening! WHATWG Request wrappers will be much easier to write 👍

Comment on lines +195 to +199
if (this[kAbortController] === undefined) {
const ac = new AbortController();
this[kAbortController] = ac;
if (this.destroyed) {
ac.abort();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

question: if a user calls message.signal on an already destroyed resources, does it make sense to go via new AbortController() + abort route?

Would it make sense to short-circuit into creating aborted Signal without the controller via AbortSignal.abort()?

This would of course require the signal to be stored on the message's symbol props instead of the controller (or along with the controller?).

This might be premature optimisation for an edge case if this doesn't happen often, though.

node --allow-natives-syntax bench.mjs
AbortController + abort                       x 321,009 ops/sec (89 runs sampled) min..max=(2.56us...3.34us)
static abort                                  x 355,118 ops/sec (93 runs sampled) min..max=(2.36us...2.84us)

Both are extremely slow, but skipping the controller is slightly faster.

bench source:

import { Suite } from 'bench-node';

const suite = new Suite({ minSamples: 100 });

suite.add('AbortController + abort', () => {
  const controller = new AbortController();
  controller.abort();
});

suite.add('static abort', () => {
  AbortSignal.abort();
});

suite.run();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The AbortSignal.abort() approach is slightly faster for the pre-destroyed case and avoids creating an unnecessary controller. I kept the current approach for simplicity ,but happy to optimize this in a follow-up if that's the preferred direction.

this.once('close', function() {
ac.abort();
});
this.once('abort', function() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

question: I'm not well versed in node.js internals, but when would this happen?
There's the deprecated "aborted" event which I assume the stdlib does not need to care about if using 'close'.
And AbortSignal does indeed use "abort" but I doubt that emitting abort on IncomingMessage is something that's expected. There's something similar on the http.ClientRequest but that's the client side aborting the outgoing request.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, we should check & test to confirm, but my expectation would be that this is never emitted for incoming messages, that's certainly what our documentation says.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The 'abort' listener was added based on @mcollina's suggestion. If it turns out this is dead code or not needed for IncomingMessage, I'm happy to remove it ,or maybe I misinterpreted the suggestion. @mcollina could you clarify the intended use case for this?

@mcollina mcollina requested a review from jasnell April 2, 2026 08:14
// Flag for when we decide that this message cannot possibly be
// read by the user, so there's no point continuing to handle it.
this._dumped = false;
this[kAbortController] = undefined;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: set to null instead to reflect object

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, updated in the latest commit

this.once('close', function() {
ac.abort();
});
this.once('abort', function() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, we should check & test to confirm, but my expectation would be that this is never emitted for incoming messages, that's certainly what our documentation says.

clientReq.on('error', () => {});
clientReq.end();
}));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the most important case here isn't covered: server receives a request & doesn't end the response (stays pending) => client aborts the request while it's incomplete (so the connection closes) => server should see req.signal.aborted == true. That's the primary end goal, allowing us to easily detect on the server side when the client cancelled the requests.

The two full-flow tests here (1 and 5) only cover serverRes.destroy() => serverReq.signal.aborted and clientReq.destroy() => clientRes.signal.aborted.

Looks like it should work, but we need to test to make sure we actually cover that integration.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks,I have added the test in the latest commit

@mcollina
Copy link
Copy Markdown
Member

mcollina commented Apr 2, 2026

This needs docs

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

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

proposal: add req.signal (AbortSignal) for automatic client disconnect detection

7 participants