Skip to content

Commit

Permalink
http2: fix socketOnTimeout and a segfault
Browse files Browse the repository at this point in the history
Fixes: nodejs/http2#179

Was fixing issue #179 and encountered a segault that was
happening somewhat randomly on session destruction. Both
should be fixed

Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <[email protected]>
Backport-Reviewed-By: Timothy Gu <[email protected]>

PR-URL: #14239
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
jasnell authored and addaleax committed Aug 14, 2017
1 parent 2620769 commit 61696f1
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 9 deletions.
29 changes: 23 additions & 6 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ function finishSessionDestroy(socket) {
debug(`[${sessionName(this[kType])}] nghttp2session handle destroyed`);
}

this.emit('close');
process.nextTick(emit.bind(this, 'close'));
debug(`[${sessionName(this[kType])}] nghttp2session destroyed`);
}

Expand Down Expand Up @@ -953,6 +953,9 @@ class Http2Session extends EventEmitter {
state.destroyed = true;
state.destroying = false;

if (this[kHandle] !== undefined)
this[kHandle].destroying();

setImmediate(finishSessionDestroy.bind(this, socket));
}

Expand Down Expand Up @@ -1985,6 +1988,7 @@ function socketDestroy(error) {
const type = this[kSession][kType];
debug(`[${sessionName(type)}] socket destroy called`);
delete this[kServer];
this.removeListener('timeout', socketOnTimeout);
// destroy the session first so that it will stop trying to
// send data while we close the socket.
this[kSession].destroy();
Expand Down Expand Up @@ -2046,14 +2050,18 @@ function socketOnError(error) {
this.destroy(error);
}

// When the socket times out, attempt a graceful shutdown
// of the session
// When the socket times out on the server, attempt a graceful shutdown
// of the session.
function socketOnTimeout() {
debug('socket timeout');
const server = this[kServer];
// server can be null if the socket is a client
if (server === undefined || !server.emit('timeout', this)) {
this[kSession].shutdown(
const session = this[kSession];
// If server or session are undefined, then we're already in the process of
// shutting down, do nothing.
if (server === undefined || session === undefined)
return;
if (!server.emit('timeout', session, this)) {
session.shutdown(
{
graceful: true,
errorCode: NGHTTP2_NO_ERROR
Expand Down Expand Up @@ -2105,6 +2113,7 @@ function connectionListener(socket) {
socket.on('resume', socketOnResume);
socket.on('pause', socketOnPause);
socket.on('drain', socketOnDrain);
socket.on('close', socketOnClose);

// Set up the Session
const session = new ServerHttp2Session(options, socket, this);
Expand Down Expand Up @@ -2197,6 +2206,13 @@ function setupCompat(ev) {
}
}

function socketOnClose(hadError) {
const session = this[kSession];
if (session !== undefined && !session.destroyed) {
session.destroy();
}
}

// If the session emits an error, forward it to the socket as a sessionError;
// failing that, destroy the session, remove the listener and re-emit the error
function clientSessionOnError(error) {
Expand Down Expand Up @@ -2244,6 +2260,7 @@ function connect(authority, options, listener) {
socket.on('resume', socketOnResume);
socket.on('pause', socketOnPause);
socket.on('drain', socketOnDrain);
socket.on('close', socketOnClose);

const session = new ClientHttp2Session(options, socket);

Expand Down
14 changes: 12 additions & 2 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -401,13 +401,21 @@ void Http2Session::Consume(const FunctionCallbackInfo<Value>& args) {
}

void Http2Session::Destroy(const FunctionCallbackInfo<Value>& args) {
DEBUG_HTTP2("Http2Session: destroying session\n");
Http2Session* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
DEBUG_HTTP2("Http2Session: destroying session %d\n", session->type());
session->Unconsume();
session->Free();
}

void Http2Session::Destroying(const FunctionCallbackInfo<Value>& args) {
Http2Session* session;
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
DEBUG_HTTP2("Http2Session: preparing to destroy session %d\n",
session->type());
session->MarkDestroying();
}

void Http2Session::SubmitPriority(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Http2Session* session;
Expand Down Expand Up @@ -816,11 +824,11 @@ void Http2Session::AllocateSend(size_t recommended, uv_buf_t* buf) {
}

void Http2Session::Send(uv_buf_t* buf, size_t length) {
DEBUG_HTTP2("Http2Session: Attempting to send data\n");
if (stream_ == nullptr || !stream_->IsAlive() || stream_->IsClosing()) {
return;
}
HandleScope scope(env()->isolate());

auto AfterWrite = [](WriteWrap* req_wrap, int status) {
req_wrap->Dispose();
};
Expand Down Expand Up @@ -1191,6 +1199,8 @@ void Initialize(Local<Object> target,
Http2Session::Consume);
env->SetProtoMethod(session, "destroy",
Http2Session::Destroy);
env->SetProtoMethod(session, "destroying",
Http2Session::Destroying);
env->SetProtoMethod(session, "sendHeaders",
Http2Session::SendHeaders);
env->SetProtoMethod(session, "submitShutdownNotice",
Expand Down
1 change: 1 addition & 0 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ class Http2Session : public AsyncWrap,
static void New(const FunctionCallbackInfo<Value>& args);
static void Consume(const FunctionCallbackInfo<Value>& args);
static void Unconsume(const FunctionCallbackInfo<Value>& args);
static void Destroying(const FunctionCallbackInfo<Value>& args);
static void Destroy(const FunctionCallbackInfo<Value>& args);
static void SubmitSettings(const FunctionCallbackInfo<Value>& args);
static void SubmitRstStream(const FunctionCallbackInfo<Value>& args);
Expand Down
12 changes: 11 additions & 1 deletion src/node_http2_core-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,12 @@ inline void Nghttp2Session::HandleGoawayFrame(const nghttp2_frame* frame) {

// Prompts nghttp2 to flush the queue of pending data frames
inline void Nghttp2Session::SendPendingData() {
DEBUG_HTTP2("Nghttp2Session %d: Sending pending data\n", session_type_);
// Do not attempt to send data on the socket if the destroying flag has
// been set. That means everything is shutting down and the socket
// will not be usable.
if (IsDestroying())
return;
const uint8_t* data;
ssize_t len = 0;
size_t ncopy = 0;
Expand Down Expand Up @@ -167,6 +173,7 @@ inline int Nghttp2Session::Init(uv_loop_t* loop,
DEBUG_HTTP2("Nghttp2Session %d: initializing session\n", type);
loop_ = loop;
session_type_ = type;
destroying_ = false;
int ret = 0;

nghttp2_session_callbacks* callbacks
Expand Down Expand Up @@ -211,6 +218,9 @@ inline int Nghttp2Session::Init(uv_loop_t* loop,
return ret;
}

inline void Nghttp2Session::MarkDestroying() {
destroying_ = true;
}

inline int Nghttp2Session::Free() {
assert(session_ != nullptr);
Expand All @@ -224,11 +234,11 @@ inline int Nghttp2Session::Free() {
session->OnFreeSession();
};
uv_close(reinterpret_cast<uv_handle_t*>(&prep_), PrepClose);

nghttp2_session_terminate_session(session_, NGHTTP2_NO_ERROR);
nghttp2_session_del(session_);
session_ = nullptr;
loop_ = nullptr;
DEBUG_HTTP2("Nghttp2Session %d: session freed\n", session_type_);
return 1;
}

Expand Down
9 changes: 9 additions & 0 deletions src/node_http2_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ class Nghttp2Session {

// Frees this session instance
inline int Free();
inline void MarkDestroying();
bool IsDestroying() {
return destroying_;
}

// Returns the pointer to the identified stream, or nullptr if
// the stream does not exist
Expand Down Expand Up @@ -128,6 +132,10 @@ class Nghttp2Session {
// Returns the nghttp2 library session
inline nghttp2_session* session() { return session_; }

nghttp2_session_type type() {
return session_type_;
}

protected:
// Adds a stream instance to this session
inline void AddStream(Nghttp2Stream* stream);
Expand Down Expand Up @@ -240,6 +248,7 @@ class Nghttp2Session {
uv_prepare_t prep_;
nghttp2_session_type session_type_;
std::unordered_map<int32_t, Nghttp2Stream*> streams_;
bool destroying_ = false;

friend class Nghttp2Stream;
};
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-http2-server-timeout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Flags: --expose-http2
'use strict';

const common = require('../common');
const http2 = require('http2');

const server = http2.createServer();
server.setTimeout(common.platformTimeout(1));

const onServerTimeout = common.mustCall((session) => {
session.destroy();
server.removeListener('timeout', onServerTimeout);
});

server.on('stream', common.mustNotCall());
server.on('timeout', onServerTimeout);

server.listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
const client = http2.connect(url);
client.on('close', common.mustCall(() => {

const client2 = http2.connect(url);
client2.on('close', common.mustCall(() => server.close()));

}));
}));

0 comments on commit 61696f1

Please sign in to comment.