Skip to content

Commit fa5a380

Browse 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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent c0d1423 commit fa5a380

17 files changed

+326
-305
lines changed

‎doc/api/errors.md

Lines changed: 13 additions & 0 deletions
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

‎doc/api/http2.md

Lines changed: 114 additions & 78 deletions
Large diffs are not rendered by default.

‎lib/internal/errors.js

Lines 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.');

‎lib/internal/http2/compat.js

Lines 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
}

‎lib/internal/http2/core.js

Lines 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 ` +

‎src/node_http2.cc

Lines 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)