Skip to content

Commit

Permalink
http2: address initial pr feedback
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@jasnell @addaleax
jasnell authored and addaleax committed Aug 14, 2017
1 parent 7824fa0 commit 26e1f8e
Show file tree
Hide file tree
Showing 18 changed files with 144 additions and 134 deletions.
11 changes: 6 additions & 5 deletions doc/api/http2.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,9 +410,10 @@ added: REPLACEME
(No Error).
* `lastStreamID` {number} The Stream ID of the last successfully processed
`Http2Stream` on this `Http2Session`.
* `opaqueData` {Buffer} A `Buffer` instance containing arbitrary additional
data to send to the peer upon disconnection. This is used, typically, to
provide additional data for debugging failures, if necessary.
* `opaqueData` {Buffer|Uint8Array} A `Buffer` or `Uint8Array` instance
containing arbitrary additional data to send to the peer upon disconnection.
This is used, typically, to provide additional data for debugging failures,
if necessary.
* `callback` {Function} A callback that is invoked after the session shutdown
has been completed.
* Returns: {undefined}
Expand Down Expand Up @@ -965,7 +966,7 @@ added: REPLACEME
initiated.
* Returns: {undefined}

Initiates a push stream. The callback is invoked with the new `Htt2Stream`
Initiates a push stream. The callback is invoked with the new `Http2Stream`
instance created for the push stream.

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

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

Expand Down
32 changes: 13 additions & 19 deletions lib/internal/http2/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

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

require('internal/util').assertCrypto();

const binding = process.binding('http2');
const debug = require('util').debuglog('http2');
const assert = require('assert');
const Buffer = require('buffer').Buffer;
const EventEmitter = require('events');
Expand All @@ -18,6 +19,8 @@ const { onServerStream } = require('internal/http2/compat');
const { utcDate } = require('internal/http');
const { _connectionListener: httpConnectionListener } = require('http');
const { isUint8Array } = process.binding('util');
const debug = util.debuglog('http2');


const {
assertIsObject,
Expand Down Expand Up @@ -459,39 +462,39 @@ function requestOnConnect(headers, options) {
}

function validatePriorityOptions(options) {
if (options.weight === undefined)
if (options.weight === undefined) {
options.weight = NGHTTP2_DEFAULT_WEIGHT;
else if (typeof options.weight !== 'number') {
} else if (typeof options.weight !== 'number') {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'weight',
options.weight);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}

if (options.parent === undefined)
if (options.parent === undefined) {
options.parent = 0;
else if (typeof options.parent !== 'number' || options.parent < 0) {
} else if (typeof options.parent !== 'number' || options.parent < 0) {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'parent',
options.parent);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}

if (options.exclusive === undefined)
if (options.exclusive === undefined) {
options.exclusive = false;
else if (typeof options.exclusive !== 'boolean') {
} else if (typeof options.exclusive !== 'boolean') {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'exclusive',
options.exclusive);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}

if (options.silent === undefined)
if (options.silent === undefined) {
options.silent = false;
else if (typeof options.silent !== 'boolean') {
} else if (typeof options.silent !== 'boolean') {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'silent',
options.silent);
Expand Down Expand Up @@ -982,7 +985,7 @@ class Http2Session extends EventEmitter {
options = Object.assign(Object.create(null), options);

if (options.opaqueData !== undefined &&
!Buffer.isBuffer(options.opaqueData)) {
!isUint8Array(options.opaqueData)) {
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'opaqueData',
options.opaqueData);
Expand All @@ -1008,13 +1011,6 @@ class Http2Session extends EventEmitter {
options.lastStreamID);
}

if (options.opaqueData !== undefined &&
!Buffer.isBuffer(options.opaqueData)) {
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'opaqueData',
options.opaqueData);
}

if (callback) {
this.on('shutdown', callback);
}
Expand Down Expand Up @@ -1233,7 +1229,6 @@ function streamOnceReady() {

function abort(stream) {
if (!stream[kState].aborted &&
stream._writableState &&
!(stream._writableState.ended || stream._writableState.ending)) {
stream.emit('aborted');
stream[kState].aborted = true;
Expand Down Expand Up @@ -1351,7 +1346,6 @@ class Http2Stream extends Duplex {
if (err)
throw util._errnoException(err, 'write', req.error);
this._bytesDispatched += req.bytes;

}

_writev(data, cb) {
Expand Down
1 change: 0 additions & 1 deletion lib/internal/http2/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ function getDefaultSettings() {

if ((flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) ===
(1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
console.log('setting it');
holder.maxConcurrentStreams =
settingsBuffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS];
}
Expand Down
4 changes: 3 additions & 1 deletion src/node.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,9 @@ NODE_EXTERN void RunAtExit(Environment* env);
v8::Isolate* isolate = target->GetIsolate(); \
v8::Local<v8::Context> context = isolate->GetCurrentContext(); \
v8::Local<v8::String> constant_name = \
v8::String::NewFromUtf8(isolate, #constant); \
v8::String::NewFromUtf8(isolate, #constant, \
v8::NewStringType::kInternalized) \
.ToLocalChecked(); \
v8::Local<v8::Number> constant_value = \
v8::Number::New(isolate, static_cast<double>(constant)); \
v8::PropertyAttribute constant_attributes = \
Expand Down
1 change: 1 addition & 0 deletions src/node_crypto_bio.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ size_t NodeBIO::IndexOf(char delim, size_t limit) {
return max;
}


void NodeBIO::Write(const char* data, size_t size) {
size_t offset = 0;
size_t left = size;
Expand Down
51 changes: 17 additions & 34 deletions src/node_http2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,33 +61,28 @@ Http2Options::Http2Options(Environment* env) {
uint32_t* buffer = env->http2_options_buffer();
uint32_t flags = buffer[IDX_OPTIONS_FLAGS];

if ((flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) ==
(1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) {
if (flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) {
SetMaxDeflateDynamicTableSize(
buffer[IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE]);
}

if ((flags & (1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) ==
(1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) {
if (flags & (1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) {
SetMaxReservedRemoteStreams(
buffer[IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS]);
}

if ((flags & (1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) ==
(1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) {
if (flags & (1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) {
SetMaxSendHeaderBlockLength(
buffer[IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH]);
}

SetPeerMaxConcurrentStreams(100); // Recommended default
if ((flags & (1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) ==
(1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) {
if (flags & (1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) {
SetPeerMaxConcurrentStreams(
buffer[IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS]);
}

if ((flags & (1 << IDX_OPTIONS_PADDING_STRATEGY)) ==
(1 << IDX_OPTIONS_PADDING_STRATEGY)) {
if (flags & (1 << IDX_OPTIONS_PADDING_STRATEGY)) {
SetPaddingStrategy(buffer[IDX_OPTIONS_PADDING_STRATEGY]);
}
}
Expand Down Expand Up @@ -197,48 +192,42 @@ void PackSettings(const FunctionCallbackInfo<Value>& args) {
uint32_t* const buffer = env->http2_settings_buffer();
uint32_t flags = buffer[IDX_SETTINGS_COUNT];

if ((flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) ==
(1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
DEBUG_HTTP2("Setting header table size: %d\n",
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE,
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) ==
(1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
if (flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
DEBUG_HTTP2("Setting max concurrent streams: %d\n",
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]);
entries.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS,
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]});
}

if ((flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) ==
(1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
if (flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
DEBUG_HTTP2("Setting max frame size: %d\n",
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_MAX_FRAME_SIZE,
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) ==
(1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
if (flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
DEBUG_HTTP2("Setting initial window size: %d\n",
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE,
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) ==
(1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
if (flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
DEBUG_HTTP2("Setting max header list size: %d\n",
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE,
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]});
}

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

if ((flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) ==
(1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) {
DEBUG_HTTP2("Setting header table size: %d\n",
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE,
buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) ==
(1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
if (flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) {
DEBUG_HTTP2("Setting max concurrent streams: %d\n",
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]);
entries.push_back({NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS,
buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]});
}

if ((flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) ==
(1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
if (flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) {
DEBUG_HTTP2("Setting max frame size: %d\n",
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_MAX_FRAME_SIZE,
buffer[IDX_SETTINGS_MAX_FRAME_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) ==
(1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
if (flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) {
DEBUG_HTTP2("Setting initial window size: %d\n",
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE,
buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) ==
(1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
if (flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) {
DEBUG_HTTP2("Setting max header list size: %d\n",
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]);
entries.push_back({NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE,
buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]});
}

if ((flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) ==
(1 << IDX_SETTINGS_ENABLE_PUSH)) {
if (flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) {
DEBUG_HTTP2("Setting enable push: %d\n",
buffer[IDX_SETTINGS_ENABLE_PUSH]);
entries.push_back({NGHTTP2_SETTINGS_ENABLE_PUSH,
Expand Down
15 changes: 7 additions & 8 deletions src/node_http2_core.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,7 @@ class Nghttp2Session {
// Removes a stream instance from this session
inline void RemoveStream(int32_t id);

virtual void Send(uv_buf_t* buf,
size_t length) {}
virtual void Send(uv_buf_t* buf, size_t length) {}
virtual void OnHeaders(Nghttp2Stream* stream,
nghttp2_header_list* headers,
nghttp2_headers_category cat,
Expand Down Expand Up @@ -294,7 +293,7 @@ class Nghttp2Stream {

// Returns true if this stream has been destroyed
inline bool IsDestroyed() const {
return (flags_ & NGHTTP2_STREAM_DESTROYED) == NGHTTP2_STREAM_DESTROYED;
return flags_ & NGHTTP2_STREAM_DESTROYED;
}

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

// Returns true if this stream is writable.
inline bool IsWritable() const {
return (flags_ & NGHTTP2_STREAM_FLAG_SHUT) == 0;
return !(flags_ & NGHTTP2_STREAM_FLAG_SHUT);
}

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

// Returns true if reading is paused
inline bool IsPaused() const {
return (flags_ & NGHTTP2_STREAM_READ_PAUSED) == NGHTTP2_STREAM_READ_PAUSED;
return flags_ & NGHTTP2_STREAM_READ_PAUSED;
}

// Returns true if this stream is in the reading state, which occurs when
// the NGHTTP2_STREAM_READ_START flag has been set and the
// NGHTTP2_STREAM_READ_PAUSED flag is *not* set.
inline bool IsReading() const {
return ((flags_ & NGHTTP2_STREAM_READ_START) == NGHTTP2_STREAM_READ_START)
&& ((flags_ & NGHTTP2_STREAM_READ_PAUSED) == 0);
return flags_ & NGHTTP2_STREAM_READ_START &&
!(flags_ & NGHTTP2_STREAM_READ_PAUSED);
}

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

// Returns the RST_STREAM code used to close this stream
Expand Down
1 change: 0 additions & 1 deletion src/stream_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,6 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
// Unref handle property
Local<Object> req_wrap_obj = req_wrap->object();
req_wrap_obj->Delete(env->context(), env->handle_string()).FromJust();

wrap->OnAfterWrite(req_wrap);

Local<Value> argv[] = {
Expand Down
6 changes: 2 additions & 4 deletions test/parallel/test-http2-client-stream-destroy-before-connect.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@ server.on('stream', (stream) => {
// must call). The error may or may not be reported depending on operating
// system specific timings.
stream.on('error', (err) => {
if (err) {
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
assert.strictEqual(err.message, 'Stream closed with error code 2');
}
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
assert.strictEqual(err.message, 'Stream closed with error code 2');
});
stream.respond({});
stream.end();
Expand Down
Loading

0 comments on commit 26e1f8e

Please sign in to comment.