Skip to content

Commit 26e1f8e

Browse files
jasnelladdaleax
jasnell
authored and
addaleax
committed
http2: address initial pr feedback
Backport-PR-URL: #14813 Backport-Reviewed-By: Anna Henningsen <[email protected]> Backport-Reviewed-By: Timothy Gu <[email protected]> PR-URL: #14239 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent 7824fa0 commit 26e1f8e

18 files changed

+144
-134
lines changed

‎doc/api/http2.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -410,9 +410,10 @@ added: REPLACEME
410410
(No Error).
411411
* `lastStreamID` {number} The Stream ID of the last successfully processed
412412
`Http2Stream` on this `Http2Session`.
413-
* `opaqueData` {Buffer} A `Buffer` instance containing arbitrary additional
414-
data to send to the peer upon disconnection. This is used, typically, to
415-
provide additional data for debugging failures, if necessary.
413+
* `opaqueData` {Buffer|Uint8Array} A `Buffer` or `Uint8Array` instance
414+
containing arbitrary additional data to send to the peer upon disconnection.
415+
This is used, typically, to provide additional data for debugging failures,
416+
if necessary.
416417
* `callback` {Function} A callback that is invoked after the session shutdown
417418
has been completed.
418419
* Returns: {undefined}
@@ -965,7 +966,7 @@ added: REPLACEME
965966
initiated.
966967
* Returns: {undefined}
967968

968-
Initiates a push stream. The callback is invoked with the new `Htt2Stream`
969+
Initiates a push stream. The callback is invoked with the new `Http2Stream`
969970
instance created for the push stream.
970971

971972
```js
@@ -1619,7 +1620,7 @@ passed in. These will always be reported by a synchronous `throw`.
16191620

16201621
State Errors occur when an action is attempted at an incorrect time (for
16211622
instance, attempting to send data on a stream after it has closed). These will
1622-
be repoorted using either a synchronous `throw` or via an `'error'` event on
1623+
be reported using either a synchronous `throw` or via an `'error'` event on
16231624
the `Http2Stream`, `Http2Session` or HTTP/2 Server objects, depending on where
16241625
and when the error occurs.
16251626

‎lib/internal/http2/core.js

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@
22

33
/* eslint-disable no-use-before-define */
44

5+
require('internal/util').assertCrypto();
6+
57
const binding = process.binding('http2');
6-
const debug = require('util').debuglog('http2');
78
const assert = require('assert');
89
const Buffer = require('buffer').Buffer;
910
const EventEmitter = require('events');
@@ -18,6 +19,8 @@ const { onServerStream } = require('internal/http2/compat');
1819
const { utcDate } = require('internal/http');
1920
const { _connectionListener: httpConnectionListener } = require('http');
2021
const { isUint8Array } = process.binding('util');
22+
const debug = util.debuglog('http2');
23+
2124

2225
const {
2326
assertIsObject,
@@ -459,39 +462,39 @@ function requestOnConnect(headers, options) {
459462
}
460463

461464
function validatePriorityOptions(options) {
462-
if (options.weight === undefined)
465+
if (options.weight === undefined) {
463466
options.weight = NGHTTP2_DEFAULT_WEIGHT;
464-
else if (typeof options.weight !== 'number') {
467+
} else if (typeof options.weight !== 'number') {
465468
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
466469
'weight',
467470
options.weight);
468471
Error.captureStackTrace(err, validatePriorityOptions);
469472
throw err;
470473
}
471474

472-
if (options.parent === undefined)
475+
if (options.parent === undefined) {
473476
options.parent = 0;
474-
else if (typeof options.parent !== 'number' || options.parent < 0) {
477+
} else if (typeof options.parent !== 'number' || options.parent < 0) {
475478
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
476479
'parent',
477480
options.parent);
478481
Error.captureStackTrace(err, validatePriorityOptions);
479482
throw err;
480483
}
481484

482-
if (options.exclusive === undefined)
485+
if (options.exclusive === undefined) {
483486
options.exclusive = false;
484-
else if (typeof options.exclusive !== 'boolean') {
487+
} else if (typeof options.exclusive !== 'boolean') {
485488
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
486489
'exclusive',
487490
options.exclusive);
488491
Error.captureStackTrace(err, validatePriorityOptions);
489492
throw err;
490493
}
491494

492-
if (options.silent === undefined)
495+
if (options.silent === undefined) {
493496
options.silent = false;
494-
else if (typeof options.silent !== 'boolean') {
497+
} else if (typeof options.silent !== 'boolean') {
495498
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
496499
'silent',
497500
options.silent);
@@ -982,7 +985,7 @@ class Http2Session extends EventEmitter {
982985
options = Object.assign(Object.create(null), options);
983986

984987
if (options.opaqueData !== undefined &&
985-
!Buffer.isBuffer(options.opaqueData)) {
988+
!isUint8Array(options.opaqueData)) {
986989
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
987990
'opaqueData',
988991
options.opaqueData);
@@ -1008,13 +1011,6 @@ class Http2Session extends EventEmitter {
10081011
options.lastStreamID);
10091012
}
10101013

1011-
if (options.opaqueData !== undefined &&
1012-
!Buffer.isBuffer(options.opaqueData)) {
1013-
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
1014-
'opaqueData',
1015-
options.opaqueData);
1016-
}
1017-
10181014
if (callback) {
10191015
this.on('shutdown', callback);
10201016
}
@@ -1233,7 +1229,6 @@ function streamOnceReady() {
12331229

12341230
function abort(stream) {
12351231
if (!stream[kState].aborted &&
1236-
stream._writableState &&
12371232
!(stream._writableState.ended || stream._writableState.ending)) {
12381233
stream.emit('aborted');
12391234
stream[kState].aborted = true;
@@ -1351,7 +1346,6 @@ class Http2Stream extends Duplex {
13511346
if (err)
13521347
throw util._errnoException(err, 'write', req.error);
13531348
this._bytesDispatched += req.bytes;
1354-
13551349
}
13561350

13571351
_writev(data, cb) {

‎lib/internal/http2/util.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,6 @@ function getDefaultSettings() {
219219

220220
if ((flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) ===
221221
(1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
222-
console.log('setting it');
223222
holder.maxConcurrentStreams =
224223
settingsBuffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS];
225224
}

‎src/node.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,9 @@ NODE_EXTERN void RunAtExit(Environment* env);
258258
v8::Isolate* isolate = target->GetIsolate(); \
259259
v8::Local<v8::Context> context = isolate->GetCurrentContext(); \
260260
v8::Local<v8::String> constant_name = \
261-
v8::String::NewFromUtf8(isolate, #constant); \
261+
v8::String::NewFromUtf8(isolate, #constant, \
262+
v8::NewStringType::kInternalized) \
263+
.ToLocalChecked(); \
262264
v8::Local<v8::Number> constant_value = \
263265
v8::Number::New(isolate, static_cast<double>(constant)); \
264266
v8::PropertyAttribute constant_attributes = \

‎src/node_crypto_bio.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,7 @@ size_t NodeBIO::IndexOf(char delim, size_t limit) {
357357
return max;
358358
}
359359

360+
360361
void NodeBIO::Write(const char* data, size_t size) {
361362
size_t offset = 0;
362363
size_t left = size;

‎src/node_http2.cc

Lines changed: 17 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -61,33 +61,28 @@ Http2Options::Http2Options(Environment* env) {
6161
uint32_t* buffer = env->http2_options_buffer();
6262
uint32_t flags = buffer[IDX_OPTIONS_FLAGS];
6363

64-
if ((flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) ==
65-
(1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) {
64+
if (flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) {
6665
SetMaxDeflateDynamicTableSize(
6766
buffer[IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE]);
6867
}
6968

70-
if ((flags & (1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) ==
71-
(1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) {
69+
if (flags & (1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) {
7270
SetMaxReservedRemoteStreams(
7371
buffer[IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS]);
7472
}
7573

76-
if ((flags & (1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) ==
77-
(1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) {
74+
if (flags & (1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) {
7875
SetMaxSendHeaderBlockLength(
7976
buffer[IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH]);
8077
}
8178

8279
SetPeerMaxConcurrentStreams(100); // Recommended default
83-
if ((flags & (1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) ==
84-
(1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) {
80+
if (flags & (1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) {
8581
SetPeerMaxConcurrentStreams(
8682
buffer[IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS]);
8783
}
8884

89-
if ((flags & (1 << IDX_OPTIONS_PADDING_STRATEGY)) ==
90-
(1 << IDX_OPTIONS_PADDING_STRATEGY)) {
85+
if (flags & (1 << IDX_OPTIONS_PADDING_STRATEGY)) {
9186
SetPaddingStrategy(buffer[IDX_OPTIONS_PADDING_STRATEGY]);
9287
}
9388
}
@@ -197,48 +192,42 @@ void PackSettings(const FunctionCallbackInfo<Value>& args) {
197192
uint32_t* const buffer = env->http2_settings_buffer();
198193
uint32_t flags = buffer[IDX_SETTINGS_COUNT];
199194

200-
if ((flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) ==
201-
(1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
195+
if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
202196
DEBUG_HTTP2("Setting header table size: %d\n",
203197
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]);
204198
entries.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE,
205199
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]});
206200
}
207201

208-
if ((flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) ==
209-
(1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
202+
if (flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
210203
DEBUG_HTTP2("Setting max concurrent streams: %d\n",
211204
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]);
212205
entries.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS,
213206
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]});
214207
}
215208

216-
if ((flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) ==
217-
(1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
209+
if (flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
218210
DEBUG_HTTP2("Setting max frame size: %d\n",
219211
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]);
220212
entries.push_back({NGHTTP2_SETTINGS_MAX_FRAME_SIZE,
221213
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]});
222214
}
223215

224-
if ((flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) ==
225-
(1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
216+
if (flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
226217
DEBUG_HTTP2("Setting initial window size: %d\n",
227218
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]);
228219
entries.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE,
229220
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]});
230221
}
231222

232-
if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) ==
233-
(1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
223+
if (flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
234224
DEBUG_HTTP2("Setting max header list size: %d\n",
235225
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]);
236226
entries.push_back({NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE,
237227
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]});
238228
}
239229

240-
if ((flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) ==
241-
(1 << IDX_SETTINGS_ENABLE_PUSH)) {
230+
if (flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) {
242231
DEBUG_HTTP2("Setting enable push: %d\n",
243232
buffer[IDX_SETTINGS_ENABLE_PUSH]);
244233
entries.push_back({NGHTTP2_SETTINGS_ENABLE_PUSH,
@@ -457,48 +446,42 @@ void Http2Session::SubmitSettings(const FunctionCallbackInfo<Value>& args) {
457446
std::vector<nghttp2_settings_entry> entries;
458447
entries.reserve(6);
459448

460-
if ((flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) ==
461-
(1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
449+
if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
462450
DEBUG_HTTP2("Setting header table size: %d\n",
463451
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]);
464452
entries.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE,
465453
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]});
466454
}
467455

468-
if ((flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) ==
469-
(1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
456+
if (flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
470457
DEBUG_HTTP2("Setting max concurrent streams: %d\n",
471458
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]);
472459
entries.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS,
473460
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]});
474461
}
475462

476-
if ((flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) ==
477-
(1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
463+
if (flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
478464
DEBUG_HTTP2("Setting max frame size: %d\n",
479465
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]);
480466
entries.push_back({NGHTTP2_SETTINGS_MAX_FRAME_SIZE,
481467
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]});
482468
}
483469

484-
if ((flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) ==
485-
(1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
470+
if (flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
486471
DEBUG_HTTP2("Setting initial window size: %d\n",
487472
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]);
488473
entries.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE,
489474
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]});
490475
}
491476

492-
if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) ==
493-
(1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
477+
if (flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
494478
DEBUG_HTTP2("Setting max header list size: %d\n",
495479
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]);
496480
entries.push_back({NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE,
497481
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]});
498482
}
499483

500-
if ((flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) ==
501-
(1 << IDX_SETTINGS_ENABLE_PUSH)) {
484+
if (flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) {
502485
DEBUG_HTTP2("Setting enable push: %d\n",
503486
buffer[IDX_SETTINGS_ENABLE_PUSH]);
504487
entries.push_back({NGHTTP2_SETTINGS_ENABLE_PUSH,

‎src/node_http2_core.h

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,7 @@ class Nghttp2Session {
143143
// Removes a stream instance from this session
144144
inline void RemoveStream(int32_t id);
145145

146-
virtual void Send(uv_buf_t* buf,
147-
size_t length) {}
146+
virtual void Send(uv_buf_t* buf, size_t length) {}
148147
virtual void OnHeaders(Nghttp2Stream* stream,
149148
nghttp2_header_list* headers,
150149
nghttp2_headers_category cat,
@@ -294,7 +293,7 @@ class Nghttp2Stream {
294293

295294
// Returns true if this stream has been destroyed
296295
inline bool IsDestroyed() const {
297-
return (flags_ & NGHTTP2_STREAM_DESTROYED) == NGHTTP2_STREAM_DESTROYED;
296+
return flags_ & NGHTTP2_STREAM_DESTROYED;
298297
}
299298

300299
// Queue outbound chunks of data to be sent on this stream
@@ -340,7 +339,7 @@ class Nghttp2Stream {
340339

341340
// Returns true if this stream is writable.
342341
inline bool IsWritable() const {
343-
return (flags_ & NGHTTP2_STREAM_FLAG_SHUT) == 0;
342+
return !(flags_ & NGHTTP2_STREAM_FLAG_SHUT);
344343
}
345344

346345
// Start Reading. If there are queued data chunks, they are pushed into
@@ -352,15 +351,15 @@ class Nghttp2Stream {
352351

353352
// Returns true if reading is paused
354353
inline bool IsPaused() const {
355-
return (flags_ & NGHTTP2_STREAM_READ_PAUSED) == NGHTTP2_STREAM_READ_PAUSED;
354+
return flags_ & NGHTTP2_STREAM_READ_PAUSED;
356355
}
357356

358357
// Returns true if this stream is in the reading state, which occurs when
359358
// the NGHTTP2_STREAM_READ_START flag has been set and the
360359
// NGHTTP2_STREAM_READ_PAUSED flag is *not* set.
361360
inline bool IsReading() const {
362-
return ((flags_ & NGHTTP2_STREAM_READ_START) == NGHTTP2_STREAM_READ_START)
363-
&& ((flags_ & NGHTTP2_STREAM_READ_PAUSED) == 0);
361+
return flags_ & NGHTTP2_STREAM_READ_START &&
362+
!(flags_ & NGHTTP2_STREAM_READ_PAUSED);
364363
}
365364

366365
inline void Close(int32_t code) {
@@ -374,7 +373,7 @@ class Nghttp2Stream {
374373
// Returns true if this stream has been closed either by receiving or
375374
// sending an RST_STREAM frame.
376375
inline bool IsClosed() const {
377-
return (flags_ & NGHTTP2_STREAM_CLOSED) == NGHTTP2_STREAM_CLOSED;
376+
return flags_ & NGHTTP2_STREAM_CLOSED;
378377
}
379378

380379
// Returns the RST_STREAM code used to close this stream

‎src/stream_base.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,6 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
408408
// Unref handle property
409409
Local<Object> req_wrap_obj = req_wrap->object();
410410
req_wrap_obj->Delete(env->context(), env->handle_string()).FromJust();
411-
412411
wrap->OnAfterWrite(req_wrap);
413412

414413
Local<Value> argv[] = {

‎test/parallel/test-http2-client-stream-destroy-before-connect.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,8 @@ server.on('stream', (stream) => {
1616
// must call). The error may or may not be reported depending on operating
1717
// system specific timings.
1818
stream.on('error', (err) => {
19-
if (err) {
20-
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
21-
assert.strictEqual(err.message, 'Stream closed with error code 2');
22-
}
19+
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
20+
assert.strictEqual(err.message, 'Stream closed with error code 2');
2321
});
2422
stream.respond({});
2523
stream.end();

0 commit comments

Comments
 (0)