| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent da1a08a commit ce0ebd8
3 files changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -870,7 +870,11 @@ function responseOnTimeout() { | |||
| 870 | 870 | function requestOnFinish() { | |
| 871 | 871 | const req = this; | |
| 872 | 872 | ||
| 873 | - if (req.shouldKeepAlive && req._ended) | ||
| 873 | + // If the response ends before this request finishes writing, `responseOnEnd()` | ||
| 874 | + // already released the socket. When `finish` fires later, that socket may | ||
| 875 | + // belong to a different request, so only call `responseKeepAlive()` when the | ||
| 876 | + // original request is still alive (`!req.destroyed`). | ||
| 877 | + if (req.shouldKeepAlive && req._ended && !req.destroyed) | ||
| 874 | 878 | responseKeepAlive(req); | |
| 875 | 879 | } | |
| 876 | 880 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,117 @@ | |||
| 1 | + 'use strict'; | ||
| 2 | + | ||
| 3 | + // Regression test for a keep-alive socket reuse race condition. | ||
| 4 | + // | ||
| 5 | + // The race is between responseOnEnd() and requestOnFinish(), both of which | ||
| 6 | + // can call responseKeepAlive(). The window is: req.end() has been called, | ||
| 7 | + // the socket write has completed (writableFinished true), but the write | ||
| 8 | + // callback that emits the 'finish' event has not fired yet. | ||
| 9 | + // | ||
| 10 | + // With plain HTTP the window is normally too narrow to hit. This test | ||
| 11 | + // widens it by delaying every client-socket write *callback* by a few | ||
| 12 | + // milliseconds (the actual I/O is not delayed, so writableFinished becomes | ||
| 13 | + // true while the 'finish'-emitting callback is still pending). | ||
| 14 | + // | ||
| 15 | + // With Expect: 100-continue, the server responds quickly while the client | ||
| 16 | + // delays req.end() just slightly (setTimeout 0), creating the perfect | ||
| 17 | + // timing for the response to arrive in that window. | ||
| 18 | + // | ||
| 19 | + // On unpatched Node, the double responseKeepAlive() call corrupts the | ||
| 20 | + // socket by stripping a subsequent request's listeners and emitting a | ||
| 21 | + // spurious 'free' event, causing requests to hang / time out. | ||
| 22 | + | ||
| 23 | + const common = require('../common'); | ||
| 24 | + const assert = require('assert'); | ||
| 25 | + const http = require('http'); | ||
| 26 | + | ||
| 27 | + const REQUEST_COUNT = 100; | ||
| 28 | + const agent = new http.Agent({ keepAlive: true, maxSockets: 1 }); | ||
| 29 | + | ||
| 30 | + // Delay every write *callback* on the client socket so that | ||
| 31 | + // socket.writableLength drops to 0 (writableFinished becomes true) before | ||
| 32 | + // the callback that ultimately emits the 'finish' event fires. With | ||
| 33 | + // HTTPS the TLS layer provides this gap naturally; for plain HTTP we | ||
| 34 | + // need to create it artificially. | ||
| 35 | + const patchedSockets = new WeakSet(); | ||
| 36 | + function patchSocket(socket) { | ||
| 37 | + if (patchedSockets.has(socket)) return; | ||
| 38 | + patchedSockets.add(socket); | ||
| 39 | + const delay = 5; | ||
| 40 | + const origWrite = socket.write; | ||
| 41 | + socket.write = function(chunk, encoding, cb) { | ||
| 42 | + if (typeof encoding === 'function') { | ||
| 43 | + cb = encoding; | ||
| 44 | + encoding = null; | ||
| 45 | + } | ||
| 46 | + if (typeof cb === 'function') { | ||
| 47 | + const orig = cb; | ||
| 48 | + cb = (...args) => setTimeout(() => orig(...args), delay); | ||
| 49 | + } | ||
| 50 | + return origWrite.call(this, chunk, encoding, cb); | ||
| 51 | + }; | ||
| 52 | + } | ||
| 53 | + | ||
| 54 | + const server = http.createServer(common.mustCall((req, res) => { | ||
| 55 | + req.on('error', common.mustNotCall()); | ||
| 56 | + res.writeHead(200); | ||
| 57 | + res.end(); | ||
| 58 | + }, REQUEST_COUNT)); | ||
| 59 | + | ||
| 60 | + server.listen(0, common.mustCall(() => { | ||
| 61 | + const { port } = server.address(); | ||
| 62 | + | ||
| 63 | + async function run() { | ||
| 64 | + try { | ||
| 65 | + for (let i = 0; i < REQUEST_COUNT; i++) { | ||
| 66 | + await sendRequest(port); | ||
| 67 | + } | ||
| 68 | + } finally { | ||
| 69 | + agent.destroy(); | ||
| 70 | + server.close(); | ||
| 71 | + } | ||
| 72 | + } | ||
| 73 | + | ||
| 74 | + run().then(common.mustCall()); | ||
| 75 | + })); | ||
| 76 | + | ||
| 77 | + function sendRequest(port) { | ||
| 78 | + let timeout; | ||
| 79 | + const promise = new Promise((resolve, reject) => { | ||
| 80 | + function done(err) { | ||
| 81 | + clearTimeout(timeout); | ||
| 82 | + if (err) | ||
| 83 | + reject(err); | ||
| 84 | + else | ||
| 85 | + resolve(); | ||
| 86 | + } | ||
| 87 | + | ||
| 88 | + const req = http.request({ | ||
| 89 | + port, | ||
| 90 | + host: '127.0.0.1', | ||
| 91 | + method: 'POST', | ||
| 92 | + agent, | ||
| 93 | + headers: { | ||
| 94 | + 'Content-Length': '0', | ||
| 95 | + 'Expect': '100-continue', | ||
| 96 | + }, | ||
| 97 | + }, common.mustCall((res) => { | ||
| 98 | + assert.strictEqual(res.statusCode, 200); | ||
| 99 | + res.resume(); | ||
| 100 | + res.once('end', done); | ||
| 101 | + res.once('error', done); | ||
| 102 | + })); | ||
| 103 | + | ||
| 104 | + req.on('socket', patchSocket); | ||
| 105 | + | ||
| 106 | + timeout = setTimeout(() => { | ||
| 107 | + const err = new Error('request timed out'); | ||
| 108 | + req.destroy(err); | ||
| 109 | + done(err); | ||
| 110 | + }, common.platformTimeout(5000)); | ||
| 111 | + | ||
| 112 | + req.once('error', done); | ||
| 113 | + | ||
| 114 | + setTimeout(() => req.end(Buffer.alloc(0)), 0); | ||
| 115 | + }); | ||
| 116 | + return promise.finally(() => clearTimeout(timeout)); | ||
| 117 | + } | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,97 @@ | |||
| 1 | + 'use strict'; | ||
| 2 | + | ||
| 3 | + // Regression test for a keep-alive socket reuse race condition. | ||
| 4 | + // | ||
| 5 | + // The race is between responseOnEnd() and requestOnFinish(), both of which | ||
| 6 | + // can call responseKeepAlive(). The window is: req.end() has been called, | ||
| 7 | + // the socket write has completed (writableFinished true), but the write | ||
| 8 | + // callback that emits the 'finish' event has not fired yet. | ||
| 9 | + // | ||
| 10 | + // HTTPS widens this window because the TLS layer introduces async | ||
| 11 | + // indirection between the actual write completion and the JS callback. | ||
| 12 | + // | ||
| 13 | + // With Expect: 100-continue, the server responds quickly while the client | ||
| 14 | + // delays req.end() just slightly (setTimeout 0), creating the perfect | ||
| 15 | + // timing for the response to arrive in that window. | ||
| 16 | + // | ||
| 17 | + // On unpatched Node, the double responseKeepAlive() call corrupts the | ||
| 18 | + // socket by stripping a subsequent request's listeners and emitting a | ||
| 19 | + // spurious 'free' event, causing requests to hang / time out. | ||
| 20 | + | ||
| 21 | + const common = require('../common'); | ||
| 22 | + | ||
| 23 | + if (!common.hasCrypto) | ||
| 24 | + common.skip('missing crypto'); | ||
| 25 | + | ||
| 26 | + const assert = require('assert'); | ||
| 27 | + const https = require('https'); | ||
| 28 | + const fixtures = require('../common/fixtures'); | ||
| 29 | + | ||
| 30 | + const REQUEST_COUNT = 100; | ||
| 31 | + const agent = new https.Agent({ keepAlive: true, maxSockets: 1 }); | ||
| 32 | + | ||
| 33 | + const key = fixtures.readKey('agent1-key.pem'); | ||
| 34 | + const cert = fixtures.readKey('agent1-cert.pem'); | ||
| 35 | + const server = https.createServer({ key, cert }, common.mustCall((req, res) => { | ||
| 36 | + req.on('error', common.mustNotCall()); | ||
| 37 | + res.writeHead(200); | ||
| 38 | + res.end(); | ||
| 39 | + }, REQUEST_COUNT)); | ||
| 40 | + | ||
| 41 | + server.listen(0, common.mustCall(() => { | ||
| 42 | + const { port } = server.address(); | ||
| 43 | + | ||
| 44 | + async function run() { | ||
| 45 | + try { | ||
| 46 | + for (let i = 0; i < REQUEST_COUNT; i++) { | ||
| 47 | + await sendRequest(port); | ||
| 48 | + } | ||
| 49 | + } finally { | ||
| 50 | + agent.destroy(); | ||
| 51 | + server.close(); | ||
| 52 | + } | ||
| 53 | + } | ||
| 54 | + | ||
| 55 | + run().then(common.mustCall()); | ||
| 56 | + })); | ||
| 57 | + | ||
| 58 | + function sendRequest(port) { | ||
| 59 | + let timeout; | ||
| 60 | + const promise = new Promise((resolve, reject) => { | ||
| 61 | + function done(err) { | ||
| 62 | + clearTimeout(timeout); | ||
| 63 | + if (err) | ||
| 64 | + reject(err); | ||
| 65 | + else | ||
| 66 | + resolve(); | ||
| 67 | + } | ||
| 68 | + | ||
| 69 | + const req = https.request({ | ||
| 70 | + port, | ||
| 71 | + host: '127.0.0.1', | ||
| 72 | + rejectUnauthorized: false, | ||
| 73 | + method: 'POST', | ||
| 74 | + agent, | ||
| 75 | + headers: { | ||
| 76 | + 'Content-Length': '0', | ||
| 77 | + 'Expect': '100-continue', | ||
| 78 | + }, | ||
| 79 | + }, common.mustCall((res) => { | ||
| 80 | + assert.strictEqual(res.statusCode, 200); | ||
| 81 | + res.resume(); | ||
| 82 | + res.once('end', done); | ||
| 83 | + res.once('error', done); | ||
| 84 | + })); | ||
| 85 | + | ||
| 86 | + timeout = setTimeout(() => { | ||
| 87 | + const err = new Error('request timed out'); | ||
| 88 | + req.destroy(err); | ||
| 89 | + done(err); | ||
| 90 | + }, common.platformTimeout(5000)); | ||
| 91 | + | ||
| 92 | + req.once('error', done); | ||
| 93 | + | ||
| 94 | + setTimeout(() => req.end(Buffer.alloc(0)), 0); | ||
| 95 | + }); | ||
| 96 | + return promise.finally(() => clearTimeout(timeout)); | ||
| 97 | + } | ||
| Back | FazBrowse Home | New Git URL |
0 commit comments