Skip to content

http: call _writeRaw callback when destroyed - #63447

Open
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:err-stream-destroyed
Open

http: call _writeRaw callback when destroyed#63447
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:err-stream-destroyed

Conversation

@trivikr

@trivikr trivikr commented May 20, 2026

Copy link
Copy Markdown
Member

OutgoingMessage._writeRaw() could return early without invoking the write
callback when the message, or its socket, was already destroyed.

This updates _writeRaw() to call the callback with ERR_STREAM_DESTROYED
in those cases, matching the existing destroyed write() behavior.

Fixes: #36673


Assisted-by: openai:gpt-5.5

@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 May 20, 2026
@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.27%. Comparing base (a6e7e6a) to head (ccfc666).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63447      +/-   ##
==========================================
- Coverage   90.30%   90.27%   -0.03%     
==========================================
  Files         760      760              
  Lines      247154   247164      +10     
  Branches    46613    46622       +9     
==========================================
- Hits       223181   223136      -45     
- Misses      15443    15500      +57     
+ Partials     8530     8528       -2     
Files with missing lines Coverage Δ
lib/_http_outgoing.js 97.96% <100.00%> (+0.33%) ⬆️

... and 37 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.

@trivikr trivikr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 20, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 20, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry pimterry left a comment

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.

Two small comments here, I could easily be persuaded to ignore these if that's complicated for some reason.

One larger issue though: with this PR in place, this code:

'use strict';
const http = require('http');
const { OutgoingMessage } = http;

const server = http.createServer((req, res) => {
  res.on('finish', () => console.log('finish'));
  res.on('error', (e) => console.log(`error:${e.code}`));
  res.on('close', () => {
    console.log('close');
    server.close();
    process.exit(0);
  });

  req.socket.destroy();
  console.log('aborted with destroy()');
  setImmediate(() => {
    // Write after socket destroyed:
    res.end('x');
    console.log('Wrote after socket destroyed');
  });
});

server.listen(0, '127.0.0.1', () => {
  http.get(`http://127.0.0.1:${server.address().port}/`).on('error', () => {});
});

Now prints:

aborted with destroy()
Wrote after socket destroyed
finish
close

I.e. with this change, we now emit 'finish' when ending aborted responses. Without this it skips finish and just closes.

I think that's because the onFinish callback used by end() ignores its error argument and just checks outmsg?.socket?._hadError, which isn't set here.

Comment thread lib/_http_outgoing.js Outdated
Comment thread lib/_http_outgoing.js Outdated
@trivikr trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 20, 2026
trivikr added 2 commits August 2, 2026 07:16
Fixes: nodejs#36673

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: openai:gpt-5.5
@trivikr
trivikr force-pushed the err-stream-destroyed branch from ce54af2 to ccfc666 Compare August 2, 2026 14:17
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.

http: OutgoingMessage ERR_STREAM_DESTROYED callback

4 participants