Skip to content

Commit

Permalink
http2: refactor method arguments to avoid bools
Browse files Browse the repository at this point in the history 
PR-URL: nodejs#15693
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
  • Loading branch information
@jasnell
jasnell committed Oct 2, 2017
1 parent 8fa0247 commit 8f4db31
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 71 deletions.
67 changes: 37 additions & 30 deletions lib/internal/http2/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ const {
HTTP_STATUS_OK,
HTTP_STATUS_NO_CONTENT,
HTTP_STATUS_NOT_MODIFIED,
HTTP_STATUS_SWITCHING_PROTOCOLS
HTTP_STATUS_SWITCHING_PROTOCOLS,

STREAM_OPTION_EMPTY_PAYLOAD,
STREAM_OPTION_GET_TRAILERS
} = constants;

// Top level to avoid creating a closure
Expand Down Expand Up @@ -412,20 +415,22 @@ function requestOnConnect(headers, options) {
return;
}

let getTrailers = false;
let streamOptions = 0;
if (options.endStream)
streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD;

if (typeof options.getTrailers === 'function') {
getTrailers = true;
streamOptions |= STREAM_OPTION_GET_TRAILERS;
this[kState].getTrailers = options.getTrailers;
}

// ret will be either the reserved stream ID (if positive)
// or an error code (if negative)
const ret = handle.submitRequest(headersList,
!!options.endStream,
streamOptions,
options.parent | 0,
options.weight | 0,
!!options.exclusive,
getTrailers);
!!options.exclusive);

// In an error condition, one of three possible response codes will be
// possible:
Expand Down Expand Up @@ -1568,7 +1573,7 @@ function processHeaders(headers) {
}

function processRespondWithFD(fd, headers, offset = 0, length = -1,
getTrailers = false) {
streamOptions = 0) {
const session = this[kSession];
const state = this[kState];
state.headersSent = true;
Expand All @@ -1578,7 +1583,7 @@ function processRespondWithFD(fd, headers, offset = 0, length = -1,

const handle = session[kHandle];
const ret =
handle.submitFile(this[kID], fd, headers, offset, length, getTrailers);
handle.submitFile(this[kID], fd, headers, offset, length, streamOptions);
let err;
switch (ret) {
case NGHTTP2_ERR_NOMEM:
Expand All @@ -1593,7 +1598,7 @@ function processRespondWithFD(fd, headers, offset = 0, length = -1,
}
}

function doSendFD(session, options, fd, headers, getTrailers, err, stat) {
function doSendFD(session, options, fd, headers, streamOptions, err, stat) {
if (this.destroyed || session.destroyed) {
abort(this);
return;
Expand Down Expand Up @@ -1623,10 +1628,10 @@ function doSendFD(session, options, fd, headers, getTrailers, err, stat) {
processRespondWithFD.call(this, fd, headersList,
statOptions.offset,
statOptions.length,
getTrailers);
streamOptions);
}

function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {
function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) {
if (this.destroyed || session.destroyed) {
abort(this);
return;
Expand Down Expand Up @@ -1681,10 +1686,10 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {
processRespondWithFD.call(this, fd, headersList,
options.offset,
options.length,
getTrailers);
streamOptions);
}

function afterOpen(session, options, headers, getTrailers, err, fd) {
function afterOpen(session, options, headers, streamOptions, err, fd) {
const state = this[kState];
const onError = options.onError;
if (this.destroyed || session.destroyed) {
Expand All @@ -1702,7 +1707,8 @@ function afterOpen(session, options, headers, getTrailers, err, fd) {
state.fd = fd;

fs.fstat(fd,
doSendFileFD.bind(this, session, options, fd, headers, getTrailers));
doSendFileFD.bind(this, session, options, fd,
headers, streamOptions));
}

function streamOnError(err) {
Expand Down Expand Up @@ -1786,9 +1792,9 @@ class ServerHttp2Stream extends Http2Stream {
throw headersList;
}

const ret = handle.submitPushPromise(this[kID],
headersList,
options.endStream);
const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0;

const ret = handle.submitPushPromise(this[kID], headersList, streamOptions);
let err;
switch (ret) {
case NGHTTP2_ERR_NOMEM:
Expand Down Expand Up @@ -1844,14 +1850,17 @@ class ServerHttp2Stream extends Http2Stream {
options = Object.assign(Object.create(null), options);
options.endStream = !!options.endStream;

let getTrailers = false;
let streamOptions = 0;
if (options.endStream)
streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD;

if (options.getTrailers !== undefined) {
if (typeof options.getTrailers !== 'function') {
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'getTrailers',
options.getTrailers);
}
getTrailers = true;
streamOptions |= STREAM_OPTION_GET_TRAILERS;
state.getTrailers = options.getTrailers;
}

Expand Down Expand Up @@ -1881,10 +1890,7 @@ class ServerHttp2Stream extends Http2Stream {

const handle = session[kHandle];
const ret =
handle.submitResponse(this[kID],
headersList,
options.endStream,
getTrailers);
handle.submitResponse(this[kID], headersList, streamOptions);
let err;
switch (ret) {
case NGHTTP2_ERR_NOMEM:
Expand Down Expand Up @@ -1937,14 +1943,14 @@ class ServerHttp2Stream extends Http2Stream {
options.statCheck);
}

let getTrailers = false;
let streamOptions = 0;
if (options.getTrailers !== undefined) {
if (typeof options.getTrailers !== 'function') {
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'getTrailers',
options.getTrailers);
}
getTrailers = true;
streamOptions |= STREAM_OPTION_GET_TRAILERS;
state.getTrailers = options.getTrailers;
}

Expand All @@ -1963,7 +1969,8 @@ class ServerHttp2Stream extends Http2Stream {

if (options.statCheck !== undefined) {
fs.fstat(fd,
doSendFD.bind(this, session, options, fd, headers, getTrailers));
doSendFD.bind(this, session, options, fd,
headers, streamOptions));
return;
}

Expand All @@ -1977,7 +1984,7 @@ class ServerHttp2Stream extends Http2Stream {
processRespondWithFD.call(this, fd, headersList,
options.offset,
options.length,
getTrailers);
streamOptions);
}

// Initiate a file response on this Http2Stream. The path is passed to
Expand Down Expand Up @@ -2019,14 +2026,14 @@ class ServerHttp2Stream extends Http2Stream {
options.statCheck);
}

let getTrailers = false;
let streamOptions = 0;
if (options.getTrailers !== undefined) {
if (typeof options.getTrailers !== 'function') {
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'getTrailers',
options.getTrailers);
}
getTrailers = true;
streamOptions |= STREAM_OPTION_GET_TRAILERS;
state.getTrailers = options.getTrailers;
}

Expand All @@ -2040,7 +2047,7 @@ class ServerHttp2Stream extends Http2Stream {
}

fs.open(path, 'r',
afterOpen.bind(this, session, options, headers, getTrailers));
afterOpen.bind(this, session, options, headers, streamOptions));
}

// Sends a block of informational headers. In theory, the HTTP/2 spec
Expand Down
32 changes: 16 additions & 16 deletions src/node_http2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ void Http2Session::SubmitRstStream(const FunctionCallbackInfo<Value>& args) {

void Http2Session::SubmitRequest(const FunctionCallbackInfo<Value>& args) {
// args[0] Array of headers
// args[1] endStream boolean
// args[1] options int
// args[2] parentStream ID (for priority spec)
// args[3] weight (for priority spec)
// args[4] exclusive boolean (for priority spec)
Expand All @@ -492,15 +492,14 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = env->isolate();

Local<Array> headers = args[0].As<Array>();
bool endStream = args[1]->BooleanValue(context).ToChecked();
int options = args[1]->IntegerValue(context).ToChecked();
int32_t parent = args[2]->Int32Value(context).ToChecked();
int32_t weight = args[3]->Int32Value(context).ToChecked();
bool exclusive = args[4]->BooleanValue(context).ToChecked();
bool getTrailers = args[5]->BooleanValue(context).ToChecked();

DEBUG_HTTP2("Http2Session: submitting request: headers: %d, end-stream: %d, "
DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d, "
"parent: %d, weight: %d, exclusive: %d\n", headers->Length(),
endStream, parent, weight, exclusive);
options, parent, weight, exclusive);

nghttp2_priority_spec prispec;
nghttp2_priority_spec_init(&prispec, parent, weight, exclusive ? 1 : 0);
Expand All @@ -509,8 +508,7 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo<Value>& args) {

int32_t ret = session->Nghttp2Session::SubmitRequest(&prispec,
*list, list.length(),
nullptr, endStream,
getTrailers);
nullptr, options);
DEBUG_HTTP2("Http2Session: request submitted, response: %d\n", ret);
args.GetReturnValue().Set(ret);
}
Expand All @@ -529,11 +527,10 @@ void Http2Session::SubmitResponse(const FunctionCallbackInfo<Value>& args) {

int32_t id = args[0]->Int32Value(context).ToChecked();
Local<Array> headers = args[1].As<Array>();
bool endStream = args[2]->BooleanValue(context).ToChecked();
bool getTrailers = args[3]->BooleanValue(context).ToChecked();
int options = args[2]->IntegerValue(context).ToChecked();

DEBUG_HTTP2("Http2Session: submitting response for stream %d: headers: %d, "
"end-stream: %d\n", id, headers->Length(), endStream);
"options: %d\n", id, headers->Length(), options);

if (!(stream = session->FindStream(id))) {
return args.GetReturnValue().Set(NGHTTP2_ERR_INVALID_STREAM_ID);
Expand All @@ -542,7 +539,7 @@ void Http2Session::SubmitResponse(const FunctionCallbackInfo<Value>& args) {
Headers list(isolate, context, headers);

args.GetReturnValue().Set(
stream->SubmitResponse(*list, list.length(), endStream, getTrailers));
stream->SubmitResponse(*list, list.length(), options));
}

void Http2Session::SubmitFile(const FunctionCallbackInfo<Value>& args) {
Expand All @@ -566,7 +563,7 @@ void Http2Session::SubmitFile(const FunctionCallbackInfo<Value>& args) {

int64_t offset = args[3]->IntegerValue(context).ToChecked();
int64_t length = args[4]->IntegerValue(context).ToChecked();
bool getTrailers = args[5]->BooleanValue(context).ToChecked();
int options = args[5]->IntegerValue(context).ToChecked();

CHECK_GE(offset, 0);

Expand All @@ -580,7 +577,7 @@ void Http2Session::SubmitFile(const FunctionCallbackInfo<Value>& args) {
Headers list(isolate, context, headers);

args.GetReturnValue().Set(stream->SubmitFile(fd, *list, list.length(),
offset, length, getTrailers));
offset, length, options));
}

void Http2Session::SendHeaders(const FunctionCallbackInfo<Value>& args) {
Expand Down Expand Up @@ -719,10 +716,10 @@ void Http2Session::SubmitPushPromise(const FunctionCallbackInfo<Value>& args) {
Nghttp2Stream* parent;
int32_t id = args[0]->Int32Value(context).ToChecked();
Local<Array> headers = args[1].As<Array>();
bool endStream = args[2]->BooleanValue(context).ToChecked();
int options = args[2]->IntegerValue(context).ToChecked();

DEBUG_HTTP2("Http2Session: submitting push promise for stream %d: "
"end-stream: %d, headers: %d\n", id, endStream,
"options: %d, headers: %d\n", id, options,
headers->Length());

if (!(parent = session->FindStream(id))) {
Expand All @@ -732,7 +729,7 @@ void Http2Session::SubmitPushPromise(const FunctionCallbackInfo<Value>& args) {
Headers list(isolate, context, headers);

int32_t ret = parent->SubmitPushPromise(*list, list.length(),
nullptr, endStream);
nullptr, options);
DEBUG_HTTP2("Http2Session: push promise submitted, ret: %d\n", ret);
args.GetReturnValue().Set(ret);
}
Expand Down Expand Up @@ -1250,6 +1247,9 @@ void Initialize(Local<Object> target,
NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_ERR_STREAM_CLOSED);
NODE_DEFINE_CONSTANT(constants, NGHTTP2_ERR_FRAME_SIZE_ERROR);

NODE_DEFINE_HIDDEN_CONSTANT(constants, STREAM_OPTION_EMPTY_PAYLOAD);
NODE_DEFINE_HIDDEN_CONSTANT(constants, STREAM_OPTION_GET_TRAILERS);

NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_NONE);
NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_END_STREAM);
NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_END_HEADERS);
Expand Down
Loading

0 comments on commit 8f4db31

Please sign in to comment.