http2: fix graceful session close · nodejs/node@a365564 · GitHub
Skip to content

Commit a365564

Browse files
pandeykushagra51RafaelGSS
pandeykushagra51
authored and
RafaelGSS
committed
http2: fix graceful session close
Fix issue where session.close() prematurely destroys the session when response.end() was called with an empty payload while active http2 streams still existed. This change ensures that sessions are closed gracefully only after all http2 streams complete and clients properly receive the GOAWAY frame as per the HTTP/2 spec. Refs: https://nodejs.org/api/http2.html\#http2sessionclosecallback PR-URL: #57808 Fixes: #57809 Refs: https://nodejs.org/api/http2.html%5C#http2sessionclosecallback Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 46fc497 commit a365564

6 files changed

Lines changed: 142 additions & 9 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 9 additions & 2 deletions

‎src/env_properties.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,7 @@
285285
V(onsignal_string, "onsignal") \
286286
V(onunpipe_string, "onunpipe") \
287287
V(onwrite_string, "onwrite") \
288+
V(ongracefulclosecomplete_string, "ongracefulclosecomplete") \
288289
V(openssl_error_stack, "opensslErrorStack") \
289290
V(options_string, "options") \
290291
V(order_string, "order") \

‎src/node_http2.cc‎

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,8 @@ Http2Session::Http2Session(Http2State* http2_state,
559559
: AsyncWrap(http2_state->env(), wrap, AsyncWrap::PROVIDER_HTTP2SESSION),
560560
js_fields_(http2_state->env()->isolate()),
561561
session_type_(type),
562-
http2_state_(http2_state) {
562+
http2_state_(http2_state),
563+
graceful_close_initiated_(false) {
563564
MakeWeak();
564565
statistics_.session_type = type;
565566
statistics_.start_time = uv_hrtime();
@@ -765,6 +766,24 @@ void Http2Stream::EmitStatistics() {
765766
});
766767
}
767768

769+
void Http2Session::HasPendingData(const FunctionCallbackInfo<Value>& args) {
770+
Http2Session* session;
771+
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
772+
args.GetReturnValue().Set(session->HasPendingData());
773+
}
774+
775+
bool Http2Session::HasPendingData() const {
776+
nghttp2_session* session = session_.get();
777+
int want_write = nghttp2_session_want_write(session);
778+
// It is expected that want_read will alway be 0 if graceful
779+
// session close is initiated and goaway frame is sent.
780+
int want_read = nghttp2_session_want_read(session);
781+
if (want_write == 0 && want_read == 0) {
782+
return false;
783+
}
784+
return true;
785+
}
786+
768787
void Http2Session::EmitStatistics() {
769788
if (!HasHttp2Observer(env())) [[likely]] {
770789
return;
@@ -1743,6 +1762,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
17431762
void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
17441763
Debug(this, "write finished with status %d", status);
17451764

1765+
MaybeNotifyGracefulCloseComplete();
17461766
CHECK(is_write_in_progress());
17471767
set_write_in_progress(false);
17481768

@@ -1965,6 +1985,7 @@ uint8_t Http2Session::SendPendingData() {
19651985
if (!res.async) {
19661986
set_write_in_progress(false);
19671987
ClearOutgoing(res.err);
1988+
MaybeNotifyGracefulCloseComplete();
19681989
}
19691990

19701991
MaybeStopReading();
@@ -3476,6 +3497,8 @@ void Initialize(Local<Object> target,
34763497
SetProtoMethod(isolate, session, "receive", Http2Session::Receive);
34773498
SetProtoMethod(isolate, session, "destroy", Http2Session::Destroy);
34783499
SetProtoMethod(isolate, session, "goaway", Http2Session::Goaway);
3500+
SetProtoMethod(
3501+
isolate, session, "hasPendingData", Http2Session::HasPendingData);
34793502
SetProtoMethod(isolate, session, "settings", Http2Session::Settings);
34803503
SetProtoMethod(isolate, session, "request", Http2Session::Request);
34813504
SetProtoMethod(
@@ -3496,6 +3519,8 @@ void Initialize(Local<Object> target,
34963519
"remoteSettings",
34973520
Http2Session::RefreshSettings<nghttp2_session_get_remote_settings,
34983521
false>);
3522+
SetProtoMethod(
3523+
isolate, session, "setGracefulClose", Http2Session::SetGracefulClose);
34993524
SetConstructorFunction(context, target, "Http2Session", session);
35003525

35013526
Local<Object> constants = Object::New(isolate);
@@ -3550,6 +3575,38 @@ void Initialize(Local<Object> target,
35503575
nghttp2_set_debug_vprintf_callback(NgHttp2Debug);
35513576
#endif
35523577
}
3578+
3579+
void Http2Session::SetGracefulClose(const FunctionCallbackInfo<Value>& args) {
3580+
Http2Session* session;
3581+
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
3582+
CHECK_NOT_NULL(session);
3583+
// Set the graceful close flag
3584+
session->SetGracefulCloseInitiated(true);
3585+
3586+
Debug(session, "Setting graceful close initiated flag");
3587+
}
3588+
3589+
void Http2Session::MaybeNotifyGracefulCloseComplete() {
3590+
nghttp2_session* session = session_.get();
3591+
3592+
if (!IsGracefulCloseInitiated()) {
3593+
return;
3594+
}
3595+
3596+
int want_write = nghttp2_session_want_write(session);
3597+
int want_read = nghttp2_session_want_read(session);
3598+
bool should_notify = (want_write == 0 && want_read == 0);
3599+
3600+
if (should_notify) {
3601+
Debug(this, "Notifying JS after write in graceful close mode");
3602+
3603+
// Make the callback to JavaScript
3604+
HandleScope scope(env()->isolate());
3605+
MakeCallback(env()->ongracefulclosecomplete_string(), 0, nullptr);
3606+
}
3607+
3608+
return;
3609+
}
35533610
} // namespace http2
35543611
} // namespace node
35553612

‎src/node_http2.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,7 @@ class Http2Session : public AsyncWrap,
712712
static void Consume(const v8::FunctionCallbackInfo<v8::Value>& args);
713713
static void Receive(const v8::FunctionCallbackInfo<v8::Value>& args);
714714
static void Destroy(const v8::FunctionCallbackInfo<v8::Value>& args);
715+
static void HasPendingData(const v8::FunctionCallbackInfo<v8::Value>& args);
715716
static void Settings(const v8::FunctionCallbackInfo<v8::Value>& args);
716717
static void Request(const v8::FunctionCallbackInfo<v8::Value>& args);
717718
static void SetNextStreamID(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -723,6 +724,7 @@ class Http2Session : public AsyncWrap,
723724
static void Ping(const v8::FunctionCallbackInfo<v8::Value>& args);
724725
static void AltSvc(const v8::FunctionCallbackInfo<v8::Value>& args);
725726
static void Origin(const v8::FunctionCallbackInfo<v8::Value>& args);
727+
static void SetGracefulClose(const v8::FunctionCallbackInfo<v8::Value>& args);
726728

727729
template <get_setting fn, bool local>
728730
static void RefreshSettings(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -735,6 +737,7 @@ class Http2Session : public AsyncWrap,
735737

736738
BaseObjectPtr<Http2Ping> PopPing();
737739
bool AddPing(const uint8_t* data, v8::Local<v8::Function> callback);
740+
bool HasPendingData() const;
738741

739742
BaseObjectPtr<Http2Settings> PopSettings();
740743
bool AddSettings(v8::Local<v8::Function> callback);
@@ -785,6 +788,13 @@ class Http2Session : public AsyncWrap,
785788

786789
Statistics statistics_ = {};
787790

791+
bool IsGracefulCloseInitiated() const {
792+
return graceful_close_initiated_;
793+
}
794+
void SetGracefulCloseInitiated(bool value) {
795+
graceful_close_initiated_ = value;
796+
}
797+
788798
private:
789799
void EmitStatistics();
790800

@@ -951,8 +961,13 @@ class Http2Session : public AsyncWrap,
951961
void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length);
952962
void ClearOutgoing(int status);
953963

964+
void MaybeNotifyGracefulCloseComplete();
965+
954966
friend class Http2Scope;
955967
friend class Http2StreamListener;
968+
969+
// Flag to indicate that JavaScript has initiated a graceful closure
970+
bool graceful_close_initiated_ = false;
956971
};
957972

958973
struct Http2SessionPerformanceEntryTraits {

‎test/parallel/test-http2-client-rststream-before-connect.js‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,23 @@ if (!common.hasCrypto)
55
common.skip('missing crypto');
66
const assert = require('assert');
77
const h2 = require('http2');
8+
let client;
89

910
const server = h2.createServer();
1011
server.on('stream', (stream) => {
11-
stream.on('close', common.mustCall());
12-
stream.respond();
13-
stream.end('ok');
12+
stream.on('close', common.mustCall(() => {
13+
client.close();
14+
server.close();
15+
}));
16+
stream.on('error', common.expectsError({
17+
code: 'ERR_HTTP2_STREAM_ERROR',
18+
name: 'Error',
19+
message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR'
20+
}));
1421
});
1522

1623
server.listen(0, common.mustCall(() => {
17-
const client = h2.connect(`http://localhost:${server.address().port}`);
24+
client = h2.connect(`http://localhost:${server.address().port}`);
1825
const req = client.request();
1926
const closeCode = 1;
2027

@@ -52,8 +59,6 @@ server.listen(0, common.mustCall(() => {
5259
req.on('close', common.mustCall(() => {
5360
assert.strictEqual(req.destroyed, true);
5461
assert.strictEqual(req.rstCode, closeCode);
55-
server.close();
56-
client.close();
5762
}));
5863

5964
req.on('error', common.expectsError({

‎test/parallel/test-http2-session-graceful-close.js‎

Lines changed: 48 additions & 0 deletions

0 commit comments

Comments
 (0)