http: add optional logging for socket timeouts - #59514
Conversation
|
Review requested:
|
mcollina
left a comment
There was a problem hiding this comment.
Thanks for sending this PR. Instead of introducing a new env variable and a console.log, please use the debuglog and NODE_DEBUG facilities.
| const serverTimeout = this.server.emit('timeout', this); | ||
|
|
||
| // Use util.debuglog for conditional logging | ||
| const debug = util.debuglog('http'); |
There was a problem hiding this comment.
move this to the top of the file
| @@ -0,0 +1,14 @@ | |||
| const assert = require('assert'); | |||
There was a problem hiding this comment.
This file is redundant and not testing anything, you can remove it
|
This pull request has been marked as stale due to 210 days of inactivity. |
|
This pull request has been marked as stale due to 90 days of inactivity. |
What does this PR do?
This PR adds optional debug logging to the
socketOnTimeoutfunction inlib/_http_server.js. The logging is enabled only when the environment variableNODE_DEBUG_TIMEOUTSis 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=falseThis 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?
_http_server.js, making it harder to trace without external tools.Changes
lib/_http_server.js: Added conditional logging insocketOnTimeout.test/parallel/test-http-timeout-logging.jsto verify logging under forced timeouts.Diff summary (full diff in commits):
Testing
make teston [your OS, e.g., Ubuntu 22.04] – all tests pass.NODE_DEBUG_TIMEOUTSis set, and timeouts are triggered correctly.benchmark/http/simple.js– performance unchanged).References
I signed the CLA. Let me know if any adjustments are needed!