Skip to content

Commit

Permalink
http: use symbol for FreeList's is_reused
Browse files Browse the repository at this point in the history
This also at least keeps up the appearance of FreeList being a generic
all purpose thing (right now it is only used for HTTP parsers but
theoretically it could be used for any reusable resource). The
previous name "needsAsyncReset" implied a tight coupling to to async
resources, the new name "is_reused" is more generic.
  • Loading branch information
basti1302 committed Oct 4, 2018
1 parent d46dc79 commit 26d1471
Show file tree
Hide file tree
Showing 9 changed files with 26 additions and 21 deletions.
2 changes: 1 addition & 1 deletion benchmark/misc/freelist.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const bench = common.createBenchmark(main, {
});

function main({ n }) {
const FreeList = require('internal/freelist');
const { FreeList } = require('internal/freelist');
const poolSize = 1000;
const list = new FreeList('test', poolSize, Object);
var j;
Expand Down
5 changes: 3 additions & 2 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const {
ERR_UNESCAPED_CHARACTERS
} = require('internal/errors').codes;
const { validateTimerDuration } = require('internal/timers');
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;

const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;

Expand Down Expand Up @@ -635,10 +636,10 @@ function tickOnSocket(req, socket) {
var parser = parsers.alloc();
req.socket = socket;
req.connection = socket;
if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) {
if (destroyHooksExist() && parser[is_reused_symbol] && parser.getAsyncId()) {
emitDestroy(parser.getAsyncId());
}
parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset);
parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
parser.socket = socket;
parser.outgoing = req;
req.parser = parser;
Expand Down
2 changes: 1 addition & 1 deletion lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

const { methods, HTTPParser } = internalBinding('http_parser');

const FreeList = require('internal/freelist');
const { FreeList } = require('internal/freelist');
const { ondrain } = require('internal/http');
const incoming = require('_http_incoming');
const {
Expand Down
5 changes: 3 additions & 2 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const {
emitDestroy,
getOrSetAsyncId
} = require('internal/async_hooks');
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
const { IncomingMessage } = require('_http_incoming');
const {
ERR_HTTP_HEADERS_SENT,
Expand Down Expand Up @@ -340,10 +341,10 @@ function connectionListenerInternal(server, socket) {
socket.on('timeout', socketOnTimeout);

var parser = parsers.alloc();
if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) {
if (destroyHooksExist() && parser[is_reused_symbol] && parser.getAsyncId()) {
emitDestroy(parser.getAsyncId());
}
parser.reinitialize(HTTPParser.REQUEST, parser.needsAsyncReset);
parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]);
parser.socket = socket;
socket.parser = parser;

Expand Down
22 changes: 12 additions & 10 deletions lib/internal/freelist.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

const is_reused_symbol = Symbol('isReused');

class FreeList {
constructor(name, max, ctor) {
this.name = name;
Expand All @@ -10,8 +12,8 @@ class FreeList {

alloc() {
return this.list.length ?
needsToCallAsyncReset(this.list.pop()) :
mustNotCallAsyncReset(this.ctor.apply(this, arguments));
setIsReused(this.list.pop(), true) :
setIsReused(this.ctor.apply(this, arguments), false);
}

free(obj) {
Expand All @@ -23,14 +25,14 @@ class FreeList {
}
}

function needsToCallAsyncReset(item) {
item.needsAsyncReset = true;
function setIsReused(item, reused) {
item[is_reused_symbol] = reused;
return item;
}

function mustNotCallAsyncReset(item) {
item.needsAsyncReset = false;
return item;
}

module.exports = FreeList;
module.exports = {
FreeList,
symbols: {
is_reused_symbol
}
};
4 changes: 2 additions & 2 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ class Parser : public AsyncWrap, public StreamListener {

CHECK(args[0]->IsInt32());
CHECK(args[1]->IsBoolean());
bool needsAsyncReset = args[1]->IsTrue();
bool isReused = args[1]->IsTrue();
http_parser_type type =
static_cast<http_parser_type>(args[0].As<Int32>()->Value());

Expand All @@ -478,7 +478,7 @@ class Parser : public AsyncWrap, public StreamListener {
// This parser has either just been created or it is being reused.
// We must only call AsyncReset for the latter case, because AsyncReset has
// already been called via the constructor for the former case.
if (needsAsyncReset) {
if (isReused) {
parser->AsyncReset();
}
parser->Init(type);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-freelist.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

require('../common');
const assert = require('assert');
const FreeList = require('internal/freelist');
const { FreeList } = require('internal/freelist');

assert.strictEqual(typeof FreeList, 'function');

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-internal-modules-expose.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ const config = process.binding('config');

console.log(config, process.argv);

assert.strictEqual(typeof require('internal/freelist'), 'function');
assert.strictEqual(typeof require('internal/freelist').FreeList, 'function');
assert.strictEqual(config.exposeInternals, true);
3 changes: 2 additions & 1 deletion test/sequential/test-http-regr-gh-2928.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const common = require('../common');
const assert = require('assert');
const httpCommon = require('_http_common');
const { internalBinding } = require('internal/test/binding');
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
const { HTTPParser } = internalBinding('http_parser');
const net = require('net');

Expand All @@ -25,7 +26,7 @@ function execAndClose() {
process.stdout.write('.');

const parser = parsers.pop();
parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset);
parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);

const socket = net.connect(common.PORT);
socket.on('error', (e) => {
Expand Down

0 comments on commit 26d1471

Please sign in to comment.