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 fa5a380

Browse filesBrowse files
jasnellBethGriggs
jasnell
authored and
BethGriggs
committed
http2: refactor how trailers are done
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. Backport-PR-URL: #22850 PR-URL: #19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent c0d1423 commit fa5a380
Copy full SHA for fa5a380
Expand file treeCollapse file tree

17 files changed

+326
-305
lines changed
Open diff view settings
Collapse file

‎doc/api/errors.md‎

Copy file name to clipboardExpand all lines: doc/api/errors.md
+13Lines changed: 13 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -861,6 +861,19 @@ When setting the priority for an HTTP/2 stream, the stream may be marked as
861861
a dependency for a parent stream. This error code is used when an attempt is
862862
made to mark a stream and dependent of itself.
863863

864+
<a id="ERR_HTTP2_TRAILERS_ALREADY_SENT"></a>
865+
### ERR_HTTP2_TRAILERS_ALREADY_SENT
866+
867+
Trailing headers have already been sent on the `Http2Stream`.
868+
869+
<a id="ERR_HTTP2_TRAILERS_NOT_READY"></a>
870+
### ERR_HTTP2_TRAILERS_NOT_READY
871+
872+
The `http2stream.sendTrailers()` method cannot be called until after the
873+
`'wantTrailers'` event is emitted on an `Http2Stream` object. The
874+
`'wantTrailers'` event will only be emitted if the `waitForTrailers` option
875+
is set for the `Http2Stream`.
876+
864877
<a id="ERR_HTTP2_UNSUPPORTED_PROTOCOL"></a>
865878
### ERR_HTTP2_UNSUPPORTED_PROTOCOL
866879

Collapse file

‎doc/api/http2.md‎

Copy file name to clipboardExpand all lines: doc/api/http2.md
+114-78Lines changed: 114 additions & 78 deletions
  • Display the source diff
  • Display the rich diff
Large diffs are not rendered by default.
Collapse file

‎lib/internal/errors.js‎

Copy file name to clipboardExpand all lines: lib/internal/errors.js
+6-2Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,8 +341,12 @@ E('ERR_HTTP2_STATUS_INVALID', 'Invalid status code: %s');
341341
E('ERR_HTTP2_STREAM_CANCEL', 'The pending stream has been canceled');
342342
E('ERR_HTTP2_STREAM_ERROR', 'Stream closed with error code %s');
343343
E('ERR_HTTP2_STREAM_SELF_DEPENDENCY', 'A stream cannot depend on itself');
344-
E('ERR_HTTP2_UNSUPPORTED_PROTOCOL',
345-
(protocol) => `protocol "${protocol}" is unsupported.`);
344+
E('ERR_HTTP2_TRAILERS_ALREADY_SENT',
345+
'Trailing headers have already been sent');
346+
E('ERR_HTTP2_TRAILERS_NOT_READY',
347+
'Trailing headers cannot be sent until after the wantTrailers event is ' +
348+
'emitted');
349+
E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.');
346350
E('ERR_HTTP_HEADERS_SENT',
347351
'Cannot render headers after they are sent to the client');
348352
E('ERR_HTTP_INVALID_CHAR', 'Invalid character in statusMessage.');
Collapse file

‎lib/internal/http2/compat.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/compat.js
+6-1Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,10 @@ class Http2ServerRequest extends Readable {
344344
}
345345
}
346346

347+
function onStreamTrailersReady() {
348+
this[kStream].sendTrailers(this[kTrailers]);
349+
}
350+
347351
class Http2ServerResponse extends Stream {
348352
constructor(stream, options) {
349353
super(options);
@@ -363,6 +367,7 @@ class Http2ServerResponse extends Stream {
363367
stream.on('drain', onStreamDrain);
364368
stream.on('aborted', onStreamAbortedResponse);
365369
stream.on('close', this[kFinish].bind(this));
370+
stream.on('wantTrailers', onStreamTrailersReady.bind(this));
366371
}
367372

368373
// User land modules such as finalhandler just check truthiness of this
@@ -632,7 +637,7 @@ class Http2ServerResponse extends Stream {
632637
headers[HTTP2_HEADER_STATUS] = state.statusCode;
633638
const options = {
634639
endStream: state.ending,
635-
getTrailers: (trailers) => Object.assign(trailers, this[kTrailers])
640+
waitForTrailers: true,
636641
};
637642
this[kStream].respond(headers, options);
638643
}
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+41-50Lines changed: 41 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -245,25 +245,18 @@ function tryClose(fd) {
245245
fs.close(fd, (err) => assert.ifError(err));
246246
}
247247

248-
// Called to determine if there are trailers to be sent at the end of a
249-
// Stream. The 'getTrailers' callback is invoked and passed a holder object.
250-
// The trailers to return are set on that object by the handler. Once the
251-
// event handler returns, those are sent off for processing. Note that this
252-
// is a necessarily synchronous operation. We need to know immediately if
253-
// there are trailing headers to send.
248+
// Called when the Http2Stream has finished sending data and is ready for
249+
// trailers to be sent. This will only be called if the { hasOptions: true }
250+
// option is set.
254251
function onStreamTrailers() {
255252
const stream = this[kOwner];
253+
stream[kState].trailersReady = true;
256254
if (stream.destroyed)
257-
return [];
258-
const trailers = Object.create(null);
259-
stream[kState].getTrailers.call(stream, trailers);
260-
const headersList = mapToHeaders(trailers, assertValidPseudoHeaderTrailer);
261-
if (!Array.isArray(headersList)) {
262-
stream.destroy(headersList);
263-
return [];
255+
return;
256+
if (!stream.emit('wantTrailers')) {
257+
// There are no listeners, send empty trailing HEADERS frame and close.
258+
stream.sendTrailers({});
264259
}
265-
stream[kSentTrailers] = trailers;
266-
return headersList;
267260
}
268261

269262
// Submit an RST-STREAM frame to be sent to the remote peer.
@@ -479,10 +472,8 @@ function requestOnConnect(headers, options) {
479472
if (options.endStream)
480473
streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD;
481474

482-
if (typeof options.getTrailers === 'function') {
475+
if (options.waitForTrailers)
483476
streamOptions |= STREAM_OPTION_GET_TRAILERS;
484-
this[kState].getTrailers = options.getTrailers;
485-
}
486477

487478
// ret will be either the reserved stream ID (if positive)
488479
// or an error code (if negative)
@@ -1367,13 +1358,6 @@ class ClientHttp2Session extends Http2Session {
13671358
options.endStream);
13681359
}
13691360

1370-
if (options.getTrailers !== undefined &&
1371-
typeof options.getTrailers !== 'function') {
1372-
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
1373-
'getTrailers',
1374-
options.getTrailers);
1375-
}
1376-
13771361
const headersList = mapToHeaders(headers);
13781362
if (!Array.isArray(headersList))
13791363
throw headersList;
@@ -1486,7 +1470,8 @@ class Http2Stream extends Duplex {
14861470
this[kState] = {
14871471
flags: STREAM_FLAGS_PENDING,
14881472
rstCode: NGHTTP2_NO_ERROR,
1489-
writeQueueSize: 0
1473+
writeQueueSize: 0,
1474+
trailersReady: false
14901475
};
14911476

14921477
this.on('resume', streamOnResume);
@@ -1742,6 +1727,33 @@ class Http2Stream extends Duplex {
17421727
priorityFn();
17431728
}
17441729

1730+
sendTrailers(headers) {
1731+
if (this.destroyed || this.closed)
1732+
throw new errors.Error('ERR_HTTP2_INVALID_STREAM');
1733+
if (this[kSentTrailers])
1734+
throw new errors.Error('ERR_HTTP2_TRAILERS_ALREADY_SENT');
1735+
if (!this[kState].trailersReady)
1736+
throw new errors.Error('ERR_HTTP2_TRAILERS_NOT_READY');
1737+
1738+
assertIsObject(headers, 'headers');
1739+
headers = Object.assign(Object.create(null), headers);
1740+
1741+
const session = this[kSession];
1742+
debug(`Http2Stream ${this[kID]} [Http2Session ` +
1743+
`${sessionName(session[kType])}]: sending trailers`);
1744+
1745+
this[kUpdateTimer]();
1746+
1747+
const headersList = mapToHeaders(headers, assertValidPseudoHeaderTrailer);
1748+
if (!Array.isArray(headersList))
1749+
throw headersList;
1750+
this[kSentTrailers] = headers;
1751+
1752+
const ret = this[kHandle].trailers(headersList);
1753+
if (ret < 0)
1754+
this.destroy(new NghttpError(ret));
1755+
}
1756+
17451757
get closed() {
17461758
return !!(this[kState].flags & STREAM_FLAGS_CLOSED);
17471759
}
@@ -2169,15 +2181,8 @@ class ServerHttp2Stream extends Http2Stream {
21692181
if (options.endStream)
21702182
streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD;
21712183

2172-
if (options.getTrailers !== undefined) {
2173-
if (typeof options.getTrailers !== 'function') {
2174-
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
2175-
'getTrailers',
2176-
options.getTrailers);
2177-
}
2184+
if (options.waitForTrailers)
21782185
streamOptions |= STREAM_OPTION_GET_TRAILERS;
2179-
state.getTrailers = options.getTrailers;
2180-
}
21812186

21822187
headers = processHeaders(headers);
21832188
const statusCode = headers[HTTP2_HEADER_STATUS] |= 0;
@@ -2243,15 +2248,8 @@ class ServerHttp2Stream extends Http2Stream {
22432248
}
22442249

22452250
let streamOptions = 0;
2246-
if (options.getTrailers !== undefined) {
2247-
if (typeof options.getTrailers !== 'function') {
2248-
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
2249-
'getTrailers',
2250-
options.getTrailers);
2251-
}
2251+
if (options.waitForTrailers)
22522252
streamOptions |= STREAM_OPTION_GET_TRAILERS;
2253-
this[kState].getTrailers = options.getTrailers;
2254-
}
22552253

22562254
if (typeof fd !== 'number')
22572255
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
@@ -2317,15 +2315,8 @@ class ServerHttp2Stream extends Http2Stream {
23172315
}
23182316

23192317
let streamOptions = 0;
2320-
if (options.getTrailers !== undefined) {
2321-
if (typeof options.getTrailers !== 'function') {
2322-
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
2323-
'getTrailers',
2324-
options.getTrailers);
2325-
}
2318+
if (options.waitForTrailers)
23262319
streamOptions |= STREAM_OPTION_GET_TRAILERS;
2327-
this[kState].getTrailers = options.getTrailers;
2328-
}
23292320

23302321
const session = this[kSession];
23312322
debug(`Http2Stream ${this[kID]} [Http2Session ` +
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+40-67Lines changed: 40 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1117,36 +1117,6 @@ inline int Http2Session::OnNghttpError(nghttp2_session* handle,
11171117
return 0;
11181118
}
11191119

1120-
// Once all of the DATA frames for a Stream have been sent, the GetTrailers
1121-
// method calls out to JavaScript to fetch the trailing headers that need
1122-
// to be sent.
1123-
inline void Http2Session::GetTrailers(Http2Stream* stream, uint32_t* flags) {
1124-
if (!stream->IsDestroyed() && stream->HasTrailers()) {
1125-
Http2Stream::SubmitTrailers submit_trailers{this, stream, flags};
1126-
stream->OnTrailers(submit_trailers);
1127-
}
1128-
}
1129-
1130-
1131-
Http2Stream::SubmitTrailers::SubmitTrailers(
1132-
Http2Session* session,
1133-
Http2Stream* stream,
1134-
uint32_t* flags)
1135-
: session_(session), stream_(stream), flags_(flags) { }
1136-
1137-
1138-
inline void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers,
1139-
size_t length) const {
1140-
Http2Scope h2scope(session_);
1141-
if (length == 0)
1142-
return;
1143-
DEBUG_HTTP2SESSION2(session_, "sending trailers for stream %d, count: %d",
1144-
stream_->id(), length);
1145-
*flags_ |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
1146-
CHECK_EQ(
1147-
nghttp2_submit_trailer(**session_, stream_->id(), trailers, length), 0);
1148-
}
1149-
11501120

11511121
// Called by OnFrameReceived to notify JavaScript land that a complete
11521122
// HEADERS frame has been received and processed. This method converts the
@@ -1808,29 +1778,6 @@ nghttp2_stream* Http2Stream::operator*() {
18081778
}
18091779

18101780

1811-
// Calls out to JavaScript land to fetch the actual trailer headers to send
1812-
// for this stream.
1813-
void Http2Stream::OnTrailers(const SubmitTrailers& submit_trailers) {
1814-
DEBUG_HTTP2STREAM(this, "prompting for trailers");
1815-
CHECK(!this->IsDestroyed());
1816-
Isolate* isolate = env()->isolate();
1817-
HandleScope scope(isolate);
1818-
Local<Context> context = env()->context();
1819-
Context::Scope context_scope(context);
1820-
1821-
Local<Value> ret =
1822-
MakeCallback(env()->ontrailers_string(), 0, nullptr).ToLocalChecked();
1823-
if (!ret.IsEmpty() && !IsDestroyed()) {
1824-
if (ret->IsArray()) {
1825-
Local<Array> headers = ret.As<Array>();
1826-
if (headers->Length() > 0) {
1827-
Headers trailers(isolate, context, headers);
1828-
submit_trailers.Submit(*trailers, trailers.length());
1829-
}
1830-
}
1831-
}
1832-
}
1833-
18341781
inline void Http2Stream::Close(int32_t code) {
18351782
CHECK(!this->IsDestroyed());
18361783
flags_ |= NGHTTP2_STREAM_FLAG_CLOSED;
@@ -1952,6 +1899,26 @@ inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) {
19521899
return ret;
19531900
}
19541901

1902+
void Http2Stream::OnTrailers() {
1903+
DEBUG_HTTP2STREAM(this, "let javascript know we are ready for trailers");
1904+
CHECK(!this->IsDestroyed());
1905+
Isolate* isolate = env()->isolate();
1906+
HandleScope scope(isolate);
1907+
Local<Context> context = env()->context();
1908+
Context::Scope context_scope(context);
1909+
MakeCallback(env()->ontrailers_string(), 0, nullptr);
1910+
}
1911+
1912+
// Submit informational headers for a stream.
1913+
int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) {
1914+
CHECK(!this->IsDestroyed());
1915+
Http2Scope h2scope(this);
1916+
DEBUG_HTTP2STREAM2(this, "sending %d trailers", len);
1917+
int ret = nghttp2_submit_trailer(**session_, id_, nva, len);
1918+
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
1919+
return ret;
1920+
}
1921+
19551922
// Submit a PRIORITY frame to the connected peer.
19561923
inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec,
19571924
bool silent) {
@@ -2184,13 +2151,6 @@ ssize_t Http2Stream::Provider::FD::OnRead(nghttp2_session* handle,
21842151
if (static_cast<size_t>(numchars) < length || length <= 0) {
21852152
DEBUG_HTTP2SESSION2(session, "no more data for stream %d", id);
21862153
*flags |= NGHTTP2_DATA_FLAG_EOF;
2187-
session->GetTrailers(stream, flags);
2188-
// If the stream or session gets destroyed during the GetTrailers
2189-
// callback, check that here and close down the stream
2190-
if (stream->IsDestroyed())
2191-
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
2192-
if (session->IsDestroyed())
2193-
return NGHTTP2_ERR_CALLBACK_FAILURE;
21942154
}
21952155

21962156
stream->statistics_.sent_bytes += numchars;
@@ -2258,13 +2218,10 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
22582218
if (stream->queue_.empty() && !stream->IsWritable()) {
22592219
DEBUG_HTTP2SESSION2(session, "no more data for stream %d", id);
22602220
*flags |= NGHTTP2_DATA_FLAG_EOF;
2261-
session->GetTrailers(stream, flags);
2262-
// If the stream or session gets destroyed during the GetTrailers
2263-
// callback, check that here and close down the stream
2264-
if (stream->IsDestroyed())
2265-
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
2266-
if (session->IsDestroyed())
2267-
return NGHTTP2_ERR_CALLBACK_FAILURE;
2221+
if (stream->HasTrailers()) {
2222+
*flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
2223+
stream->OnTrailers();
2224+
}
22682225
}
22692226

22702227
stream->statistics_.sent_bytes += amount;
@@ -2574,6 +2531,21 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& args) {
25742531
headers->Length());
25752532
}
25762533

2534+
// Submits trailing headers on the Http2Stream
2535+
void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& args) {
2536+
Environment* env = Environment::GetCurrent(args);
2537+
Local<Context> context = env->context();
2538+
Isolate* isolate = env->isolate();
2539+
Http2Stream* stream;
2540+
ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder());
2541+
2542+
Local<Array> headers = args[0].As<Array>();
2543+
2544+
Headers list(isolate, context, headers);
2545+
args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length()));
2546+
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length());
2547+
}
2548+
25772549
// Grab the numeric id of the Http2Stream
25782550
void Http2Stream::GetID(const FunctionCallbackInfo<Value>& args) {
25792551
Http2Stream* stream;
@@ -2921,6 +2893,7 @@ void Initialize(Local<Object> target,
29212893
env->SetProtoMethod(stream, "priority", Http2Stream::Priority);
29222894
env->SetProtoMethod(stream, "pushPromise", Http2Stream::PushPromise);
29232895
env->SetProtoMethod(stream, "info", Http2Stream::Info);
2896+
env->SetProtoMethod(stream, "trailers", Http2Stream::Trailers);
29242897
env->SetProtoMethod(stream, "respondFD", Http2Stream::RespondFD);
29252898
env->SetProtoMethod(stream, "respond", Http2Stream::Respond);
29262899
env->SetProtoMethod(stream, "rstStream", Http2Stream::RstStream);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.