http: revert #14024 writable is never set to false

Setting writable = false in IncomingMessage.end made some errors
being swallowed in some very popular OSS libraries that we must
support. This commit add some of those use cases to the tests,
so we avoid further regressions.
We should reevaluate how to set writable = false in IncomingMessage
in a way that does not break the ecosystem.

See: nodejs/node#14024
Fixes: nodejs/node#15029
PR-URL: nodejs/node#15404
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>

Author: mcollina

lib/_http_outgoing.js

@@ -780,7 +780,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
     this.connection.uncork();
 
   this.finished = true;
-  this.writable = false;
 
   // There is the first message on the outgoing queue, and we've sent
   // everything to the socket.

test/parallel/test-http-outgoing-finish-writable.js

@@ -4,13 +4,16 @@ const assert = require('assert');
 const http = require('http');
 
 // Verify that after calling end() on an `OutgoingMessage` (or a type that
-// inherits from `OutgoingMessage`), its `writable` property is set to false.
+// inherits from `OutgoingMessage`), its `writable` property is not set to false
 
 const server = http.createServer(common.mustCall(function(req, res) {
   assert.strictEqual(res.writable, true);
   assert.strictEqual(res.finished, false);
   res.end();
-  assert.strictEqual(res.writable, false);
+
+  // res.writable is set to false after it has finished sending
+  // Ref: https://github.com/nodejs/node/issues/15029
+  assert.strictEqual(res.writable, true);
   assert.strictEqual(res.finished, true);
 
   server.close();
@@ -27,5 +30,9 @@ server.on('listening', common.mustCall(function() {
 
   assert.strictEqual(clientRequest.writable, true);
   clientRequest.end();
-  assert.strictEqual(clientRequest.writable, false);
+
+  // writable is still true when close
+  // THIS IS LEGACY, we cannot change it
+  // unless we break error detection
+  assert.strictEqual(clientRequest.writable, true);
 }));

test/parallel/test-http-writable-true-after-close.js

@@ -0,0 +1,35 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { get, createServer } = require('http');
+
+// res.writable should not be set to false after it has finished sending
+// Ref: https://github.com/nodejs/node/issues/15029
+
+let external;
+
+// Http server
+const internal = createServer((req, res) => {
+  res.writeHead(200);
+  setImmediate(common.mustCall(() => {
+    external.abort();
+    res.end('Hello World\n');
+  }));
+}).listen(0);
+
+// Proxy server
+const server = createServer(common.mustCall((req, res) => {
+  get(`http://127.0.0.1:${internal.address().port}`, common.mustCall((inner) => {
+    res.on('close', common.mustCall(() => {
+      assert.strictEqual(res.writable, true);
+    }));
+    inner.pipe(res);
+  }));
+})).listen(0, () => {
+  external = get(`http://127.0.0.1:${server.address().port}`);
+  external.on('error', common.mustCall((err) => {
+    server.close();
+    internal.close();
+  }));
+});

test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js

@@ -1,14 +1,13 @@
 'use strict';
 const common = require('../common');
 const http = require('http');
+const assert = require('assert');
 const util = require('util');
 const stream = require('stream');
 
 // Verify that when piping a stream to an `OutgoingMessage` (or a type that
 // inherits from `OutgoingMessage`), if data is emitted after the
-// `OutgoingMessage` was closed - no `write after end` error is raised (this
-// should be the case when piping - when writing data directly to the
-// `OutgoingMessage` this error should be raised).
+// `OutgoingMessage` was closed - a `write after end` error is raised
 
 function MyStream() {
   stream.call(this);
@@ -22,8 +21,10 @@ const server = http.createServer(common.mustCall(function(req, res) {
   process.nextTick(common.mustCall(() => {
     res.end();
     myStream.emit('data', 'some data');
+    res.on('error', common.mustCall(function(err) {
+      assert.strictEqual(err.message, 'write after end');
+    }));
 
-    // If we got here - 'write after end' wasn't raised and the test passed.
     process.nextTick(common.mustCall(() => server.close()));
   }));
 }));