http: revert #14024 writable is never set to false

mcollina · jasnell · commit 771c2ac7c295 · 2017-09-20T07:52:19.000-07:00
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: #14024 Fixes: #15029 PR-URL: #15404 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
@@ -769,7 +769,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.
diff --git a/test/parallel/test-http-outgoing-finish-writable.js b/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);
 }));
diff --git a/test/parallel/test-http-writable-true-after-close.js b/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();
+  }));
+});
diff --git a/test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js b/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()));
   }));
 }));
