FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: fix graceful session close · nodejs/node@1377173 · GitHub

/ node Public

Commit 1377173

Browse files
pandeykushagra51
authored and
aduh95
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 c956d37 commit 1377173

6 files changed

Lines changed: 142 additions & 9 deletions

File tree

‎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]) {
@@ -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

‎src/env_properties.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,7 @@
276276
V(onsignal_string, "onsignal") \
277277
V(onunpipe_string, "onunpipe") \
278278
V(onwrite_string, "onwrite") \
279+
V(ongracefulclosecomplete_string, "ongracefulclosecomplete") \
279280
V(openssl_error_stack, "opensslErrorStack") \
280281
V(options_string, "options") \
281282
V(order_string, "order") \

‎src/node_http2.cc‎

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,8 @@ Http2Session::Http2Session(Http2State* http2_state,
561561
: AsyncWrap(http2_state->env(), wrap, AsyncWrap::PROVIDER_HTTP2SESSION),
562562
js_fields_(http2_state->env()->isolate()),
563563
session_type_(type),
564-
http2_state_(http2_state) {
564+
http2_state_(http2_state),
565+
graceful_close_initiated_(false) {
565566
MakeWeak();
566567
statistics_.session_type = type;
567568
statistics_.start_time = uv_hrtime();
@@ -767,6 +768,24 @@ void Http2Stream::EmitStatistics() {
767768
});
768769
}
769770

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

1767+
MaybeNotifyGracefulCloseComplete();
17481768
CHECK(is_write_in_progress());
17491769
set_write_in_progress(false);
17501770

@@ -1967,6 +1987,7 @@ uint8_t Http2Session::SendPendingData() {
19671987
if (!res.async) {
19681988
set_write_in_progress(false);
19691989
ClearOutgoing(res.err);
1990+
MaybeNotifyGracefulCloseComplete();
19701991
}
19711992

19721993
MaybeStopReading();
@@ -3478,6 +3499,8 @@ void Initialize(Local<Object> target,
34783499
SetProtoMethod(isolate, session, "receive", Http2Session::Receive);
34793500
SetProtoMethod(isolate, session, "destroy", Http2Session::Destroy);
34803501
SetProtoMethod(isolate, session, "goaway", Http2Session::Goaway);
3502+
SetProtoMethod(
3503+
isolate, session, "hasPendingData", Http2Session::HasPendingData);
34813504
SetProtoMethod(isolate, session, "settings", Http2Session::Settings);
34823505
SetProtoMethod(isolate, session, "request", Http2Session::Request);
34833506
SetProtoMethod(
@@ -3498,6 +3521,8 @@ void Initialize(Local<Object> target,
34983521
"remoteSettings",
34993522
Http2Session::RefreshSettings<nghttp2_session_get_remote_settings,
35003523
false>);
3524+
SetProtoMethod(
3525+
isolate, session, "setGracefulClose", Http2Session::SetGracefulClose);
35013526
SetConstructorFunction(context, target, "Http2Session", session);
35023527

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

‎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)
Back | FazBrowse Home | New Git URL