Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit a365564

Browse filesBrowse 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
Copy full SHA for a365564

File tree

Expand file treeCollapse file tree

6 files changed

+142
-9
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+142
-9
lines changed
Open diff view settings
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+9-2Lines 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]) {
@@ -1589,6 +1590,10 @@ class Http2Session extends EventEmitter {
15891590
if (typeof callback === 'function')
15901591
this.once('close', callback);
15911592
this.goaway();
1593+
const handle = this[kHandle];
1594+
if (handle) {
1595+
handle.setGracefulClose();
1596+
}
15921597
this[kMaybeDestroy]();
15931598
}
15941599

@@ -1609,11 +1614,13 @@ class Http2Session extends EventEmitter {
16091614
// * session is closed and there are no more pending or open streams
16101615
[kMaybeDestroy](error) {
16111616
if (error == null) {
1617+
const handle = this[kHandle];
1618+
const hasPendingData = !!handle && handle.hasPendingData();
16121619
const state = this[kState];
16131620
// Do not destroy if we're not closed and there are pending/open streams
16141621
if (!this.closed ||
16151622
state.streams.size > 0 ||
1616-
state.pendingStreams.size > 0) {
1623+
state.pendingStreams.size > 0 || hasPendingData) {
16171624
return;
16181625
}
16191626
}
@@ -3300,7 +3307,7 @@ function socketOnClose() {
33003307
state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
33013308
state.pendingStreams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
33023309
session.close();
3303-
session[kMaybeDestroy](err);
3310+
closeSession(session, NGHTTP2_NO_ERROR, err);
33043311
}
33053312
}
33063313

Collapse file

‎src/env_properties.h‎

Copy file name to clipboardExpand all lines: src/env_properties.h
+1Lines 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") \
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+58-1Lines 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

Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+15Lines 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 {
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http2-client-rststream-before-connect.js
+11-6Lines 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({
Collapse file

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

Copy file name to clipboard
+48Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.