http2: fix socketOnTimeout and a segfault
Fixes: https://github.com/nodejs/http2/issues/179 Was fixing issue #179 and encountered a segault that was happening somewhat randomly on session destruction. Both should be fixed PR-URL: https://github.com/nodejs/node/pull/14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This commit is contained in:
parent
6911fe337c
commit
953458f645
@ -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`);
|
||||
}
|
||||
|
||||
@ -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));
|
||||
}
|
||||
|
||||
@ -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();
|
||||
@ -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
|
||||
@ -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);
|
||||
@ -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) {
|
||||
@ -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);
|
||||
|
||||
|
@ -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;
|
||||
@ -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();
|
||||
};
|
||||
@ -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",
|
||||
|
@ -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);
|
||||
|
@ -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;
|
||||
@ -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
|
||||
@ -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);
|
||||
@ -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;
|
||||
}
|
||||
|
||||
|
@ -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
|
||||
@ -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);
|
||||
@ -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;
|
||||
};
|
||||
|
27
test/parallel/test-http2-server-timeout.js
Executable file
27
test/parallel/test-http2-server-timeout.js
Executable file
@ -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()));
|
||||
|
||||
}));
|
||||
}));
|
Loading…
x
Reference in New Issue
Block a user