X Tutup
The Wayback Machine - https://web.archive.org/web/20250718034826/https://github.com/nodejs/node/commit/bbd5aec785
Skip to content

Commit bbd5aec

Browse files
pandeykushagra51marco-ippolito
pandeykushagra51
authored and
marco-ippolito
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 49e624f commit bbd5aec

File tree

6 files changed

+142
-9
lines changed

6 files changed

+142
-9
lines changed

‎lib/internal/http2/core.js

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,6 +1068,7 @@ function setupHandle(socket, type, options) {
10681068
if (typeof options.selectPadding === 'function')
10691069
this[kSelectPadding] = options.selectPadding;
10701070
handle.consume(socket._handle);
1071+
handle.ongracefulclosecomplete = this[kMaybeDestroy].bind(this, null);
10711072

10721073
this[kHandle] = handle;
10731074
if (this[kNativeFields]) {
@@ -1592,6 +1593,10 @@ class Http2Session extends EventEmitter {
15921593
if (typeof callback === 'function')
15931594
this.once('close', callback);
15941595
this.goaway();
1596+
const handle = this[kHandle];
1597+
if (handle) {
1598+
handle.setGracefulClose();
1599+
}
15951600
this[kMaybeDestroy]();
15961601
}
15971602

@@ -1612,11 +1617,13 @@ class Http2Session extends EventEmitter {
16121617
// * session is closed and there are no more pending or open streams
16131618
[kMaybeDestroy](error) {
16141619
if (error == null) {
1620+
const handle = this[kHandle];
1621+
const hasPendingData = !!handle && handle.hasPendingData();
16151622
const state = this[kState];
16161623
// Do not destroy if we're not closed and there are pending/open streams
16171624
if (!this.closed ||
16181625
state.streams.size > 0 ||
1619-
state.pendingStreams.size > 0) {
1626+
state.pendingStreams.size > 0 || hasPendingData) {
16201627
return;
16211628
}
16221629
}
@@ -3310,7 +3317,7 @@ function socketOnClose() {
33103317
state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
33113318
state.pendingStreams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
33123319
session.close();
3313-
session[kMaybeDestroy](err);
3320+
closeSession(session, NGHTTP2_NO_ERROR, err);
33143321
}
33153322
}
33163323

‎src/env_properties.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@
249249
V(onsignal_string, "onsignal") \
250250
V(onunpipe_string, "onunpipe") \
251251
V(onwrite_string, "onwrite") \
252+
V(ongracefulclosecomplete_string, "ongracefulclosecomplete") \
252253
V(openssl_error_stack, "opensslErrorStack") \
253254
V(options_string, "options") \
254255
V(order_string, "order") \

‎src/node_http2.cc

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,8 @@ Http2Session::Http2Session(Http2State* http2_state,
545545
: AsyncWrap(http2_state->env(), wrap, AsyncWrap::PROVIDER_HTTP2SESSION),
546546
js_fields_(http2_state->env()->isolate()),
547547
session_type_(type),
548-
http2_state_(http2_state) {
548+
http2_state_(http2_state),
549+
graceful_close_initiated_(false) {
549550
MakeWeak();
550551
statistics_.session_type = type;
551552
statistics_.start_time = uv_hrtime();
@@ -749,6 +750,24 @@ void Http2Stream::EmitStatistics() {
749750
});
750751
}
751752

753+
void Http2Session::HasPendingData(const FunctionCallbackInfo<Value>& args) {
754+
Http2Session* session;
755+
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
756+
args.GetReturnValue().Set(session->HasPendingData());
757+
}
758+
759+
bool Http2Session::HasPendingData() const {
760+
nghttp2_session* session = session_.get();
761+
int want_write = nghttp2_session_want_write(session);
762+
// It is expected that want_read will alway be 0 if graceful
763+
// session close is initiated and goaway frame is sent.
764+
int want_read = nghttp2_session_want_read(session);
765+
if (want_write == 0 && want_read == 0) {
766+
return false;
767+
}
768+
return true;
769+
}
770+
752771
void Http2Session::EmitStatistics() {
753772
if (LIKELY(!HasHttp2Observer(env())))
754773
return;
@@ -1725,6 +1744,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
17251744
void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
17261745
Debug(this, "write finished with status %d", status);
17271746

1747+
MaybeNotifyGracefulCloseComplete();
17281748
CHECK(is_write_in_progress());
17291749
set_write_in_progress(false);
17301750

@@ -1945,6 +1965,7 @@ uint8_t Http2Session::SendPendingData() {
19451965
if (!res.async) {
19461966
set_write_in_progress(false);
19471967
ClearOutgoing(res.err);
1968+
MaybeNotifyGracefulCloseComplete();
19481969
}
19491970

19501971
MaybeStopReading();
@@ -3418,6 +3439,8 @@ void Initialize(Local<Object> target,
34183439
SetProtoMethod(isolate, session, "receive", Http2Session::Receive);
34193440
SetProtoMethod(isolate, session, "destroy", Http2Session::Destroy);
34203441
SetProtoMethod(isolate, session, "goaway", Http2Session::Goaway);
3442+
SetProtoMethod(
3443+
isolate, session, "hasPendingData", Http2Session::HasPendingData);
34213444
SetProtoMethod(isolate, session, "settings", Http2Session::Settings);
34223445
SetProtoMethod(isolate, session, "request", Http2Session::Request);
34233446
SetProtoMethod(
@@ -3438,6 +3461,8 @@ void Initialize(Local<Object> target,
34383461
"remoteSettings",
34393462
Http2Session::RefreshSettings<nghttp2_session_get_remote_settings,
34403463
false>);
3464+
SetProtoMethod(
3465+
isolate, session, "setGracefulClose", Http2Session::SetGracefulClose);
34413466
SetConstructorFunction(context, target, "Http2Session", session);
34423467

34433468
Local<Object> constants = Object::New(isolate);
@@ -3492,6 +3517,38 @@ void Initialize(Local<Object> target,
34923517
nghttp2_set_debug_vprintf_callback(NgHttp2Debug);
34933518
#endif
34943519
}
3520+
3521+
void Http2Session::SetGracefulClose(const FunctionCallbackInfo<Value>& args) {
3522+
Http2Session* session;
3523+
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
3524+
CHECK_NOT_NULL(session);
3525+
// Set the graceful close flag
3526+
session->SetGracefulCloseInitiated(true);
3527+
3528+
Debug(session, "Setting graceful close initiated flag");
3529+
}
3530+
3531+
void Http2Session::MaybeNotifyGracefulCloseComplete() {
3532+
nghttp2_session* session = session_.get();
3533+
3534+
if (!IsGracefulCloseInitiated()) {
3535+
return;
3536+
}
3537+
3538+
int want_write = nghttp2_session_want_write(session);
3539+
int want_read = nghttp2_session_want_read(session);
3540+
bool should_notify = (want_write == 0 && want_read == 0);
3541+
3542+
if (should_notify) {
3543+
Debug(this, "Notifying JS after write in graceful close mode");
3544+
3545+
// Make the callback to JavaScript
3546+
HandleScope scope(env()->isolate());
3547+
MakeCallback(env()->ongracefulclosecomplete_string(), 0, nullptr);
3548+
}
3549+
3550+
return;
3551+
}
34953552
} // namespace http2
34963553
} // namespace node
34973554

‎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
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
const assert = require('assert');
7+
const h2 = require('http2');
8+
9+
const server = h2.createServer();
10+
let session;
11+
12+
server.on('session', common.mustCall(function(s) {
13+
session = s;
14+
session.on('close', common.mustCall(function() {
15+
server.close();
16+
}));
17+
}));
18+
19+
server.listen(0, common.mustCall(function() {
20+
const port = server.address().port;
21+
22+
const url = `http://localhost:${port}`;
23+
const client = h2.connect(url, common.mustCall(function() {
24+
const headers = {
25+
':path': '/',
26+
':method': 'GET',
27+
':scheme': 'http',
28+
':authority': `localhost:${port}`
29+
};
30+
const request = client.request(headers);
31+
request.on('response', common.mustCall(function(headers) {
32+
assert.strictEqual(headers[':status'], 200);
33+
}, 1));
34+
request.on('end', common.mustCall(function() {
35+
client.close();
36+
}));
37+
request.end();
38+
request.resume();
39+
}));
40+
client.on('goaway', common.mustCallAtLeast(1));
41+
}));
42+
43+
server.once('request', common.mustCall(function(request, response) {
44+
response.on('finish', common.mustCall(function() {
45+
session.close();
46+
}));
47+
response.end();
48+
}));

0 commit comments

Comments
 (0)
X Tutup