Skip to content

Commit

Permalink
http2: fix issues with aborted respondWithFile()s
Browse files Browse the repository at this point in the history
Fixes: #20824
Fixes: #21560
  • Loading branch information
addaleax committed Jul 15, 2018
1 parent 1009118 commit bb4e94a
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 9 deletions.
3 changes: 1 addition & 2 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
11 changes: 8 additions & 3 deletions src/stream_pipe.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
}

Expand Down Expand Up @@ -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();
}
Expand Down
10 changes: 6 additions & 4 deletions src/stream_pipe.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
30 changes: 30 additions & 0 deletions test/parallel/test-http2-respond-with-file-connection-abort.js
Original file line number Diff line number Diff line change
@@ -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();
}));

0 comments on commit bb4e94a

Please sign in to comment.