Skip to content

http: add optional logging for socket timeouts#59514

Open
rohit-chouhan wants to merge 8 commits intonodejs:mainfrom
rohit-chouhan:main
Open

http: add optional logging for socket timeouts#59514
rohit-chouhan wants to merge 8 commits intonodejs:mainfrom
rohit-chouhan:main

Conversation

@rohit-chouhan
Copy link
Copy Markdown

What does this PR do?

This PR adds optional debug logging to the socketOnTimeout function in lib/_http_server.js. The logging is enabled only when the environment variable NODE_DEBUG_TIMEOUTS is set (e.g., NODE_DEBUG_TIMEOUTS=1 node app.js). It outputs details like whether the timeout affected the request, response, or server, helping debug issues like unexpected connection drops.

Example log output:
Socket timeout: req=true, res=false, server=false

This change is non-breaking, has no performance impact in production (since it's gated by an env var), and aligns with Node.js's debuglog patterns.

Why is this useful?

Changes

  • Updated lib/_http_server.js: Added conditional logging in socketOnTimeout.
  • Added new test: test/parallel/test-http-timeout-logging.js to verify logging under forced timeouts.

Diff summary (full diff in commits):

function socketOnTimeout() {
    // Existing code...
    if (process.env.NODE_DEBUG_TIMEOUTS) {
		console.log(Socket timeout: req=${!!req}, res=${!!res}, server=${!!serverTimeout});
    }
    // Existing code...
}

Testing

  • Ran make test on [your OS, e.g., Ubuntu 22.04] – all tests pass.
  • Manually tested with a simple HTTP server: Logging appears only when NODE_DEBUG_TIMEOUTS is set, and timeouts are triggered correctly.
  • No regressions in benchmark/http/ (ran benchmark/http/simple.js – performance unchanged).

References

I signed the CLA. Let me know if any adjustments are needed!

@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 Aug 18, 2025
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.

Thanks for sending this PR. Instead of introducing a new env variable and a console.log, please use the debuglog and NODE_DEBUG facilities.

@rohit-chouhan rohit-chouhan requested a review from mcollina August 18, 2025 08:05
Comment thread lib/_http_server.js Outdated
const serverTimeout = this.server.emit('timeout', this);

// Use util.debuglog for conditional logging
const debug = util.debuglog('http');
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.

move this to the top of the file

@@ -0,0 +1,14 @@
const assert = require('assert');
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.

This file is redundant and not testing anything, you can remove it

@rohit-chouhan rohit-chouhan requested a review from mcollina August 18, 2025 08:58
@rohit-chouhan
Copy link
Copy Markdown
Author

@mcollina

@github-actions
Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions bot added the stale label Apr 19, 2026
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. stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants