http: fix just-merged change that could silently truncate responses

lib/_http_client.js

@@ -753,16 +753,15 @@ function socketErrorListener(err) {
 
   if (req) {
     const res = req.res;
+    const exchangeComplete = req.writableFinished && res?.complete;
     const isUserDestroyError = err === req[kError] || err === res?.errored;
 
     // For Safety. Some additional errors might fire later on
     // and we need to make sure we don't double-fire the error event.
     socket._hadError = true;
-    // Before a response exists, the request itself failed. Once a response
-    // exists, socket teardown belongs to the IncomingMessage and is finalized
-    // by socketCloseListener. Preserve errors explicitly used to destroy the
-    // request or response, which have historically been emitted on the request.
-    if (!res || isUserDestroyError) {
+    // Once both messages are complete, a transport error cannot change the
+    // result of the exchange. User-provided destroy errors are always emitted.
+    if (!exchangeComplete || isUserDestroyError) {
       emitErrorEvent(req, err);
     }
 

test/parallel/test-http-client-complete-response-open-request-reset.js

@@ -0,0 +1,42 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const BODY = Buffer.alloc(1024 * 1024);
+
+const server = http.createServer(common.mustCall((request, response) => {
+  response.writeHead(202, { 'content-length': 0 });
+  response.end();
+
+  request.once('data', common.mustCall(() => {
+    request.socket.resetAndDestroy();
+  }));
+}));
+
+server.on('clientError', common.mustNotCall());
+
+server.listen(0, common.mustCall(() => {
+  let response;
+  const req = http.request({
+    method: 'POST',
+    port: server.address().port,
+    headers: { 'content-length': BODY.length },
+  }, common.mustCall((res) => {
+    response = res;
+    assert.strictEqual(res.statusCode, 202);
+    assert.strictEqual(req.writableEnded, false);
+    req.write(BODY);
+  }));
+
+  req.on('error', common.mustCall((err) => {
+    assert(response);
+    assert.strictEqual(response.complete, true);
+    assert.strictEqual(req.writableFinished, false);
+    assert.strictEqual(err.code, 'ECONNRESET');
+  }));
+
+  req.on('close', common.mustCall(() => server.close()));
+  req.flushHeaders();
+}));

test/parallel/test-http-client-complete-response-reset.js

@@ -1,3 +1,3 @@
 'use strict';
 
 const common = require('../common');
@@ -11,13 +11,14 @@
 
   request.on('data', function onData(chunk) {
     received += chunk.length;
-    request.off('data', onData);
-    request.destroy();
+    if (received > BODY.length / 2) {
+      request.off('data', onData);
+      response.writeHead(413, { 'content-length': 0 });
+      response.end();
+      request.destroy();
+    }
   });
 
-  response.writeHead(413, { 'content-length': 0 });
-  response.end();
-
   request.on('end', common.mustNotCall());
   request.on('close', common.mustCall(() => {
     assert.strictEqual(request.complete, false);
@@ -38,18 +39,17 @@
     response = res;
     assert.strictEqual(res.statusCode, 413);
     // Deliberately do not consume the response body. A response that has
-    // already been received should not be followed by a late ClientRequest
-    // socket error.
-    req.write(BODY);
-    req.end();
+    // completed after the request finished should not be followed by a late
+    // ClientRequest socket error.
   }));
 
   req.on('error', common.mustNotCall());
   req.on('close', common.mustCall(() => {
     assert(response);
+    assert.strictEqual(req.writableFinished, true);
     assert.strictEqual(response.complete, true);
     server.close();
   }));
 
-  req.flushHeaders();
+  req.end(BODY);
 }));

test/parallel/test-http-client-incomplete-response-reset.js

@@ -0,0 +1,57 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+// A response that is cut short by a connection reset must still surface the
+// socket error on the request. The response exists, but it is not complete, so
+// swallowing the error would leave the application with a silently truncated
+// body it believes is intact.
+
+const LENGTH = 1024;
+
+let serverSocket;
+
+const server = http.createServer(common.mustCall((request, response) => {
+  serverSocket = request.socket;
+  // Promise more body than is ever sent.
+  response.writeHead(200, { 'content-length': LENGTH });
+  response.write('hello');
+}));
+
+server.on('clientError', common.mustNotCall());
+
+server.listen(0, common.mustCall(() => {
+  const req = http.request({ port: server.address().port }, common.mustCall((res) => {
+    assert.strictEqual(res.statusCode, 200);
+
+    let received = 0;
+    let reset = false;
+
+    res.on('data', common.mustCallAtLeast((chunk) => {
+      received += chunk.length;
+      if (reset) return;
+      reset = true;
+      // The headers and part of the body have arrived. Reset from the server
+      // side so the client sees a genuine inbound RST mid-body.
+      serverSocket.resetAndDestroy();
+    }, 1));
+
+    res.on('end', common.mustNotCall());
+    res.on('close', common.mustCall(() => {
+      assert.strictEqual(res.complete, false);
+      assert.strictEqual(res.errored.code, 'ECONNRESET');
+      assert.strictEqual(res.errored.message, 'aborted');
+      assert.ok(received > 0 && received < LENGTH,
+                `expected a truncated body, got ${received} of ${LENGTH}`);
+      server.close();
+    }));
+  }));
+
+  req.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ECONNRESET');
+  }));
+
+  req.end();
+}));