http2: fix issues with aborted respondWithFile()s

Fixes: #20824 Fixes: #21560
nodejs · addaleax · Jun 27, 2018 · Jul 15, 2018 · Jul 15, 2018 · bb4e94aa012644a1ed6688be8ca891a09f753cc2
commit bb4e94aa012644a1ed6688be8ca891a09f753cc2
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -2060,10 +2060,9 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap,
                          uv_buf_t* bufs,
                          size_t nbufs,
                          uv_stream_t* send_handle) {
-  CHECK(!this->IsDestroyed());
   CHECK_NULL(send_handle);
   Http2Scope h2scope(this);
-  if (!IsWritable()) {
+  if (!IsWritable() || IsDestroyed()) {
     req_wrap->Done(UV_EOF);
     return 0;
   }

diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc
@@ -57,9 +57,11 @@ void StreamPipe::Unpipe() {
   if (is_closed_)
     return;
 
-  // Note that we cannot use virtual methods on `source` and `sink` here,
-  // because this function can be called from their destructors via
+  // Note that we possibly cannot use virtual methods on `source` and `sink`
+  // here, because this function can be called from their destructors via
   // `OnStreamDestroy()`.
+  if (!source_destroyed_)
+    source()->ReadStop();
 
   is_closed_ = true;
   is_reading_ = false;
@@ -144,7 +146,8 @@ void StreamPipe::ProcessData(size_t nread, const uv_buf_t& buf) {
     is_writing_ = true;
     is_reading_ = false;
     res.wrap->SetAllocatedStorage(buf.base, buf.len);
-    source()->ReadStop();
+    if (source() != nullptr)
+      source()->ReadStop();
   }
 }
 
@@ -183,13 +186,15 @@ void StreamPipe::WritableListener::OnStreamAfterShutdown(ShutdownWrap* w,
 
 void StreamPipe::ReadableListener::OnStreamDestroy() {
   StreamPipe* pipe = ContainerOf(&StreamPipe::readable_listener_, this);
+  pipe->source_destroyed_ = true;
   if (!pipe->is_eof_) {
     OnStreamRead(UV_EPIPE, uv_buf_init(nullptr, 0));
   }
 }
 
 void StreamPipe::WritableListener::OnStreamDestroy() {
   StreamPipe* pipe = ContainerOf(&StreamPipe::writable_listener_, this);
+  pipe->sink_destroyed_ = true;
   pipe->is_eof_ = true;
   pipe->Unpipe();
 }

diff --git a/src/stream_pipe.h b/src/stream_pipe.h
@@ -23,16 +23,18 @@ class StreamPipe : public AsyncWrap {
   }
 
  private:
-  StreamBase* source();
-  StreamBase* sink();
+  inline StreamBase* source();
+  inline StreamBase* sink();
 
-  void ShutdownWritable();
-  void FlushToWritable();
+  inline void ShutdownWritable();
+  inline void FlushToWritable();
 
   bool is_reading_ = false;
   bool is_writing_ = false;
   bool is_eof_ = false;
   bool is_closed_ = true;
+  bool sink_destroyed_ = false;
+  bool source_destroyed_ = false;
 
   // Set a default value so that when we’re coming from Start(), we know
   // that we don’t want to read just yet.

diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js
@@ -0,0 +1,30 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const net = require('net');
+
+const {
+  HTTP2_HEADER_CONTENT_TYPE
+} = http2.constants;
+
+const server = http2.createServer();
+server.on('stream', common.mustCall((stream) => {
+  stream.respondWithFile(process.execPath, {
+    [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream'
+  });
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('response', common.mustCall(() => {}));
+  req.on('data', common.mustCall(() => {
+    net.Socket.prototype.destroy.call(client.socket);
+    server.close();
+  }));
+  req.end();
+}));