Skip to content

Commit 5eca84b

Browse files
addaleaxMylesBorins
addaleax
authored and
MylesBorins
committed
http2: only schedule write when necessary
Introduce an `Http2Scope` class that, when it goes out of scope, checks whether a write to the network is desired by nghttp2. If that is the case, schedule a write using `SetImmediate()` rather than a custom per-session libuv handle. PR-URL: #17183 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
1 parent 2cfa6b4 commit 5eca84b

3 files changed

Lines changed: 114 additions & 41 deletions

File tree

‎src/node_http2.cc‎

Lines changed: 61 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,26 @@ const Http2Session::Callbacks Http2Session::callback_struct_saved[2] = {
2727
Callbacks(false),
2828
Callbacks(true)};
2929

30+
Http2Scope::Http2Scope(Http2Stream* stream) : Http2Scope(stream->session()) {}
31+
32+
Http2Scope::Http2Scope(Http2Session* session) {
33+
if (session->flags_ & (SESSION_STATE_HAS_SCOPE |
34+
SESSION_STATE_WRITE_SCHEDULED)) {
35+
// There is another scope further below on the stack, or it is already
36+
// known that a write is scheduled. In either case, there is nothing to do.
37+
return;
38+
}
39+
session->flags_ |= SESSION_STATE_HAS_SCOPE;
40+
session_ = session;
41+
}
42+
43+
Http2Scope::~Http2Scope() {
44+
if (session_ == nullptr)
45+
return;
46+
47+
session_->flags_ &= ~SESSION_STATE_HAS_SCOPE;
48+
session_->MaybeScheduleWrite();
49+
}
3050

3151
Http2Options::Http2Options(Environment* env) {
3252
nghttp2_option_new(&options_);
@@ -346,8 +366,6 @@ Http2Session::Http2Session(Environment* env,
346366
// be catching before it gets this far. Either way, crash if this
347367
// fails.
348368
CHECK_EQ(fn(&session_, callbacks, this, *opts), 0);
349-
350-
Start();
351369
}
352370

353371

@@ -356,40 +374,6 @@ Http2Session::~Http2Session() {
356374
Close();
357375
}
358376

359-
// For every node::Http2Session instance, there is a uv_prepare_t handle
360-
// whose callback is triggered on every tick of the event loop. When
361-
// run, nghttp2 is prompted to send any queued data it may have stored.
362-
// TODO(jasnell): Currently, this creates one uv_prepare_t per Http2Session,
363-
// we should investigate to see if it's faster to create a
364-
// single uv_prepare_t for all Http2Sessions, then iterate
365-
// over each.
366-
void Http2Session::Start() {
367-
prep_ = new uv_prepare_t();
368-
uv_prepare_init(env()->event_loop(), prep_);
369-
prep_->data = static_cast<void*>(this);
370-
uv_prepare_start(prep_, [](uv_prepare_t* t) {
371-
Http2Session* session = static_cast<Http2Session*>(t->data);
372-
HandleScope scope(session->env()->isolate());
373-
Context::Scope context_scope(session->env()->context());
374-
375-
// Sending data may call arbitrary JS code, so keep track of
376-
// async context.
377-
InternalCallbackScope callback_scope(session);
378-
session->SendPendingData();
379-
});
380-
}
381-
382-
// Stop the uv_prep_t from further activity, destroy the handle
383-
void Http2Session::Stop() {
384-
DEBUG_HTTP2SESSION(this, "stopping uv_prep_t handle");
385-
CHECK_EQ(uv_prepare_stop(prep_), 0);
386-
auto prep_close = [](uv_handle_t* handle) {
387-
delete reinterpret_cast<uv_prepare_t*>(handle);
388-
};
389-
uv_close(reinterpret_cast<uv_handle_t*>(prep_), prep_close);
390-
prep_ = nullptr;
391-
}
392-
393377

394378
void Http2Session::Close() {
395379
DEBUG_HTTP2SESSION(this, "closing session");
@@ -408,8 +392,6 @@ void Http2Session::Close() {
408392
Http2Session::Http2Ping* ping = PopPing();
409393
ping->Done(false);
410394
}
411-
412-
Stop();
413395
}
414396

415397

@@ -480,6 +462,7 @@ inline void Http2Session::SubmitShutdownNotice() {
480462
inline void Http2Session::Settings(const nghttp2_settings_entry iv[],
481463
size_t niv) {
482464
DEBUG_HTTP2SESSION2(this, "submitting %d settings", niv);
465+
Http2Scope h2scope(this);
483466
// This will fail either if the system is out of memory, or if the settings
484467
// values are not within the appropriate range. We should be catching the
485468
// latter before it gets this far so crash in either case.
@@ -732,7 +715,8 @@ Http2Stream::SubmitTrailers::SubmitTrailers(
732715

733716

734717
inline void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers,
735-
size_t length) const {
718+
size_t length) const {
719+
Http2Scope h2scope(session_);
736720
if (length == 0)
737721
return;
738722
DEBUG_HTTP2SESSION2(session_, "sending trailers for stream %d, count: %d",
@@ -887,14 +871,37 @@ inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
887871
MakeCallback(env()->onsettings_string(), arraysize(argv), argv);
888872
}
889873

874+
void Http2Session::MaybeScheduleWrite() {
875+
CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0);
876+
if (session_ != nullptr && nghttp2_session_want_write(session_)) {
877+
flags_ |= SESSION_STATE_WRITE_SCHEDULED;
878+
env()->SetImmediate([](Environment* env, void* data) {
879+
Http2Session* session = static_cast<Http2Session*>(data);
880+
if (session->session_ == nullptr ||
881+
!(session->flags_ & SESSION_STATE_WRITE_SCHEDULED)) {
882+
// This can happen e.g. when a stream was reset before this turn
883+
// of the event loop, in which case SendPendingData() is called early,
884+
// or the session was destroyed in the meantime.
885+
return;
886+
}
887+
888+
// Sending data may call arbitrary JS code, so keep track of
889+
// async context.
890+
InternalCallbackScope callback_scope(session);
891+
session->SendPendingData();
892+
}, static_cast<void*>(this), object());
893+
}
894+
}
895+
890896

891-
inline void Http2Session::SendPendingData() {
897+
void Http2Session::SendPendingData() {
892898
DEBUG_HTTP2SESSION(this, "sending pending data");
893899
// Do not attempt to send data on the socket if the destroying flag has
894900
// been set. That means everything is shutting down and the socket
895901
// will not be usable.
896902
if (IsDestroying())
897903
return;
904+
flags_ &= ~SESSION_STATE_WRITE_SCHEDULED;
898905

899906
WriteWrap* req = nullptr;
900907
char* dest = nullptr;
@@ -959,6 +966,7 @@ inline Http2Stream* Http2Session::SubmitRequest(
959966
int32_t* ret,
960967
int options) {
961968
DEBUG_HTTP2SESSION(this, "submitting request");
969+
Http2Scope h2scope(this);
962970
Http2Stream* stream = nullptr;
963971
Http2Stream::Provider::Stream prov(options);
964972
*ret = nghttp2_submit_request(session_, prispec, nva, len, *prov, nullptr);
@@ -1018,6 +1026,7 @@ void Http2Session::OnStreamReadImpl(ssize_t nread,
10181026
uv_handle_type pending,
10191027
void* ctx) {
10201028
Http2Session* session = static_cast<Http2Session*>(ctx);
1029+
Http2Scope h2scope(session);
10211030
if (nread < 0) {
10221031
uv_buf_t tmp_buf;
10231032
tmp_buf.base = nullptr;
@@ -1183,6 +1192,7 @@ inline void Http2Stream::Close(int32_t code) {
11831192

11841193

11851194
inline void Http2Stream::Shutdown() {
1195+
Http2Scope h2scope(this);
11861196
flags_ |= NGHTTP2_STREAM_FLAG_SHUT;
11871197
CHECK_NE(nghttp2_session_resume_data(session_->session(), id_),
11881198
NGHTTP2_ERR_NOMEM);
@@ -1197,6 +1207,7 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) {
11971207
}
11981208

11991209
inline void Http2Stream::Destroy() {
1210+
Http2Scope h2scope(this);
12001211
DEBUG_HTTP2STREAM(this, "destroying stream");
12011212
// Do nothing if this stream instance is already destroyed
12021213
if (IsDestroyed())
@@ -1248,6 +1259,7 @@ void Http2Stream::OnDataChunk(
12481259

12491260

12501261
inline void Http2Stream::FlushDataChunks() {
1262+
Http2Scope h2scope(this);
12511263
if (!data_chunks_.empty()) {
12521264
uv_buf_t buf = data_chunks_.front();
12531265
data_chunks_.pop();
@@ -1265,6 +1277,7 @@ inline void Http2Stream::FlushDataChunks() {
12651277
inline int Http2Stream::SubmitResponse(nghttp2_nv* nva,
12661278
size_t len,
12671279
int options) {
1280+
Http2Scope h2scope(this);
12681281
DEBUG_HTTP2STREAM(this, "submitting response");
12691282
if (options & STREAM_OPTION_GET_TRAILERS)
12701283
flags_ |= NGHTTP2_STREAM_FLAG_TRAILERS;
@@ -1285,6 +1298,7 @@ inline int Http2Stream::SubmitFile(int fd,
12851298
int64_t offset,
12861299
int64_t length,
12871300
int options) {
1301+
Http2Scope h2scope(this);
12881302
DEBUG_HTTP2STREAM(this, "submitting file");
12891303
if (options & STREAM_OPTION_GET_TRAILERS)
12901304
flags_ |= NGHTTP2_STREAM_FLAG_TRAILERS;
@@ -1301,6 +1315,7 @@ inline int Http2Stream::SubmitFile(int fd,
13011315

13021316
// Submit informational headers for a stream.
13031317
inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) {
1318+
Http2Scope h2scope(this);
13041319
DEBUG_HTTP2STREAM2(this, "sending %d informational headers", len);
13051320
int ret = nghttp2_submit_headers(session_->session(),
13061321
NGHTTP2_FLAG_NONE,
@@ -1313,6 +1328,7 @@ inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) {
13131328

13141329
inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec,
13151330
bool silent) {
1331+
Http2Scope h2scope(this);
13161332
DEBUG_HTTP2STREAM(this, "sending priority spec");
13171333
int ret = silent ?
13181334
nghttp2_session_change_stream_priority(session_->session(),
@@ -1326,6 +1342,7 @@ inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec,
13261342

13271343

13281344
inline int Http2Stream::SubmitRstStream(const uint32_t code) {
1345+
Http2Scope h2scope(this);
13291346
DEBUG_HTTP2STREAM2(this, "sending rst-stream with code %d", code);
13301347
session_->SendPendingData();
13311348
CHECK_EQ(nghttp2_submit_rst_stream(session_->session(),
@@ -1341,6 +1358,7 @@ inline Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva,
13411358
size_t len,
13421359
int32_t* ret,
13431360
int options) {
1361+
Http2Scope h2scope(this);
13441362
DEBUG_HTTP2STREAM(this, "sending push promise");
13451363
*ret = nghttp2_submit_push_promise(session_->session(), NGHTTP2_FLAG_NONE,
13461364
id_, nva, len, nullptr);
@@ -1380,6 +1398,7 @@ inline int Http2Stream::Write(nghttp2_stream_write_t* req,
13801398
const uv_buf_t bufs[],
13811399
unsigned int nbufs,
13821400
nghttp2_stream_write_cb cb) {
1401+
Http2Scope h2scope(this);
13831402
if (!IsWritable()) {
13841403
if (cb != nullptr)
13851404
cb(req, UV_EOF);
@@ -1763,6 +1782,7 @@ void Http2Session::Goaway(const FunctionCallbackInfo<Value>& args) {
17631782
Environment* env = Environment::GetCurrent(args);
17641783
Local<Context> context = env->context();
17651784
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
1785+
Http2Scope h2scope(session);
17661786

17671787
uint32_t errorCode = args[0]->Uint32Value(context).ToChecked();
17681788
int32_t lastStreamID = args[1]->Int32Value(context).ToChecked();
@@ -2038,6 +2058,7 @@ void Http2Session::Http2Ping::Send(uint8_t* payload) {
20382058
memcpy(&data, &startTime_, arraysize(data));
20392059
payload = data;
20402060
}
2061+
Http2Scope h2scope(session_);
20412062
CHECK_EQ(nghttp2_submit_ping(**session_, NGHTTP2_FLAG_NONE, payload), 0);
20422063
}
20432064

‎src/node_http2.h‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,9 @@ const char* nghttp2_errname(int rv) {
417417

418418
enum session_state_flags {
419419
SESSION_STATE_NONE = 0x0,
420-
SESSION_STATE_DESTROYING = 0x1
420+
SESSION_STATE_DESTROYING = 0x1,
421+
SESSION_STATE_HAS_SCOPE = 0x2,
422+
SESSION_STATE_WRITE_SCHEDULED = 0x4
421423
};
422424

423425
// This allows for 4 default-sized frames with their frame headers
@@ -429,6 +431,19 @@ typedef uint32_t(*get_setting)(nghttp2_session* session,
429431
class Http2Session;
430432
class Http2Stream;
431433

434+
// This scope should be present when any call into nghttp2 that may schedule
435+
// data to be written to the underlying transport is made, and schedules
436+
// such a write automatically once the scope is exited.
437+
class Http2Scope {
438+
public:
439+
explicit Http2Scope(Http2Stream* stream);
440+
explicit Http2Scope(Http2Session* session);
441+
~Http2Scope();
442+
443+
private:
444+
Http2Session* session_ = nullptr;
445+
};
446+
432447
// The Http2Options class is used to parse the options object passed in to
433448
// a Http2Session object and convert those into an appropriate nghttp2_option
434449
// struct. This is the primary mechanism by which the Http2Session object is
@@ -816,6 +831,9 @@ class Http2Session : public AsyncWrap {
816831
inline void MarkDestroying() { flags_ |= SESSION_STATE_DESTROYING; }
817832
inline bool IsDestroying() { return flags_ & SESSION_STATE_DESTROYING; }
818833

834+
// Schedule a write if nghttp2 indicates it wants to write to the socket.
835+
void MaybeScheduleWrite();
836+
819837
// Returns pointer to the stream, or nullptr if stream does not exist
820838
inline Http2Stream* FindStream(int32_t id);
821839

@@ -1005,6 +1023,8 @@ class Http2Session : public AsyncWrap {
10051023

10061024
size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
10071025
std::queue<Http2Ping*> outstanding_pings_;
1026+
1027+
friend class Http2Scope;
10081028
};
10091029

10101030
class Http2Session::Http2Ping : public AsyncWrap {

‎test/parallel/test-http2-session-gc-while-write-scheduled.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// Flags: --expose-gc
2+
3+
'use strict';
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const http2 = require('http2');
8+
const makeDuplexPair = require('../common/duplexpair');
9+
10+
// This tests that running garbage collection while an Http2Session has
11+
// a write *scheduled*, it will survive that garbage collection.
12+
13+
{
14+
// This creates a session and schedules a write (for the settings frame).
15+
let client = http2.connect('http://localhost:80', {
16+
createConnection: common.mustCall(() => makeDuplexPair().clientSide)
17+
});
18+
19+
// First, wait for any nextTicks() and their responses
20+
// from the `connect()` call to run.
21+
tick(10, () => {
22+
// This schedules a write.
23+
client.settings(http2.getDefaultSettings());
24+
client = null;
25+
global.gc();
26+
});
27+
}
28+
29+
function tick(n, cb) {
30+
if (n--) setImmediate(tick, n, cb);
31+
else cb();
32+
}

0 commit comments

Comments
 (0)