Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: simple code refactoring #11594

Closed
wants to merge 2 commits into from
Closed

http: simple code refactoring #11594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fe6aab
http: use more efficient module.exports pattern
jasnell Feb 28, 2017
a51ae7d
http: replace uses of self
jasnell Feb 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/_http_agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ function Agent(options) {
}

util.inherits(Agent, EventEmitter);
exports.Agent = Agent;

Agent.defaultMaxSockets = Infinity;

Expand Down Expand Up @@ -314,4 +313,7 @@ Agent.prototype.destroy = function destroy() {
}
};

exports.globalAgent = new Agent();
module.exports = {
Agent,
globalAgent: new Agent()
};
144 changes: 68 additions & 76 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ function isInvalidPath(s) {
}

function ClientRequest(options, cb) {
var self = this;
OutgoingMessage.call(self);
OutgoingMessage.call(this);

if (typeof options === 'string') {
options = url.parse(options);
Expand Down Expand Up @@ -95,12 +94,12 @@ function ClientRequest(options, cb) {
'Agent option must be an instance of http.Agent, undefined or false.'
);
}
self.agent = agent;
this.agent = agent;

var protocol = options.protocol || defaultAgent.protocol;
var expectedProtocol = defaultAgent.protocol;
if (self.agent && self.agent.protocol)
expectedProtocol = self.agent.protocol;
if (this.agent && this.agent.protocol)
expectedProtocol = this.agent.protocol;

var path;
if (options.path) {
Expand All @@ -121,15 +120,15 @@ function ClientRequest(options, cb) {
}

const defaultPort = options.defaultPort ||
self.agent && self.agent.defaultPort;
this.agent && this.agent.defaultPort;

var port = options.port = options.port || defaultPort || 80;
var host = options.host = options.hostname || options.host || 'localhost';

var setHost = (options.setHost === undefined);

self.socketPath = options.socketPath;
self.timeout = options.timeout;
this.socketPath = options.socketPath;
this.timeout = options.timeout;

var method = options.method;
var methodIsString = (typeof method === 'string');
Expand All @@ -141,14 +140,14 @@ function ClientRequest(options, cb) {
if (!common._checkIsHttpToken(method)) {
throw new TypeError('Method must be a valid HTTP token');
}
method = self.method = method.toUpperCase();
method = this.method = method.toUpperCase();
} else {
method = self.method = 'GET';
method = this.method = 'GET';
}

self.path = options.path || '/';
this.path = options.path || '/';
if (cb) {
self.once('response', cb);
this.once('response', cb);
}

var headersArray = Array.isArray(options.headers);
Expand All @@ -157,7 +156,7 @@ function ClientRequest(options, cb) {
var keys = Object.keys(options.headers);
for (var i = 0; i < keys.length; i++) {
var key = keys[i];
self.setHeader(key, options.headers[key]);
this.setHeader(key, options.headers[key]);
}
}
if (host && !this.getHeader('host') && setHost) {
Expand Down Expand Up @@ -190,21 +189,22 @@ function ClientRequest(options, cb) {
method === 'DELETE' ||
method === 'OPTIONS' ||
method === 'CONNECT') {
self.useChunkedEncodingByDefault = false;
this.useChunkedEncodingByDefault = false;
} else {
self.useChunkedEncodingByDefault = true;
this.useChunkedEncodingByDefault = true;
}

if (headersArray) {
self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n',
this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n',
options.headers);
} else if (self.getHeader('expect')) {
if (self._header) {
} else if (this.getHeader('expect')) {
if (this._header) {
throw new Error('Can\'t render headers after they are sent to the ' +
'client');
}
self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n',
self[outHeadersKey]);

this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n',
this[outHeadersKey]);
}

this._ended = false;
Expand All @@ -216,77 +216,69 @@ function ClientRequest(options, cb) {
this.maxHeadersCount = null;

var called = false;
if (self.socketPath) {
self._last = true;
self.shouldKeepAlive = false;

const oncreate = (err, socket) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change from function declaration to variable declaration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function was using a closure around self. I modified it to use lexical this with the arrow function.

if (called)
return;
called = true;
if (err) {
process.nextTick(() => this.emit('error', err));
return;
}
this.onSocket(socket);
this._deferToConnect(null, null, () => this._flush());
};

if (this.socketPath) {
this._last = true;
this.shouldKeepAlive = false;
const optionsPath = {
path: self.socketPath,
timeout: self.timeout
path: this.socketPath,
timeout: this.timeout
};
const newSocket = self.agent.createConnection(optionsPath, oncreate);
const newSocket = this.agent.createConnection(optionsPath, oncreate);
if (newSocket && !called) {
called = true;
self.onSocket(newSocket);
this.onSocket(newSocket);
} else {
return;
}
} else if (self.agent) {
} else if (this.agent) {
// If there is an agent we should default to Connection:keep-alive,
// but only if the Agent will actually reuse the connection!
// If it's not a keepAlive agent, and the maxSockets==Infinity, then
// there's never a case where this socket will actually be reused
if (!self.agent.keepAlive && !Number.isFinite(self.agent.maxSockets)) {
self._last = true;
self.shouldKeepAlive = false;
if (!this.agent.keepAlive && !Number.isFinite(this.agent.maxSockets)) {
this._last = true;
this.shouldKeepAlive = false;
} else {
self._last = false;
self.shouldKeepAlive = true;
this._last = false;
this.shouldKeepAlive = true;
}
self.agent.addRequest(self, options);
this.agent.addRequest(this, options);
} else {
// No agent, default to Connection:close.
self._last = true;
self.shouldKeepAlive = false;
this._last = true;
this.shouldKeepAlive = false;
if (typeof options.createConnection === 'function') {
const newSocket = options.createConnection(options, oncreate);
if (newSocket && !called) {
called = true;
self.onSocket(newSocket);
this.onSocket(newSocket);
} else {
return;
}
} else {
debug('CLIENT use net.createConnection', options);
self.onSocket(net.createConnection(options));
}
}

function oncreate(err, socket) {
if (called)
return;
called = true;
if (err) {
process.nextTick(function() {
self.emit('error', err);
});
return;
this.onSocket(net.createConnection(options));
}
self.onSocket(socket);
self._deferToConnect(null, null, function() {
self._flush();
self = null;
});
}

self._deferToConnect(null, null, function() {
self._flush();
self = null;
});
this._deferToConnect(null, null, () => this._flush());
}

util.inherits(ClientRequest, OutgoingMessage);

exports.ClientRequest = ClientRequest;

ClientRequest.prototype._finish = function _finish() {
DTRACE_HTTP_CLIENT_REQUEST(this, this.connection);
Expand All @@ -305,7 +297,7 @@ ClientRequest.prototype._implicitHeader = function _implicitHeader() {

ClientRequest.prototype.abort = function abort() {
if (!this.aborted) {
process.nextTick(emitAbortNT, this);
process.nextTick(emitAbortNT.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use .bind() in any of these cleanup PRs, we won't be able to/shouldn't backport to v6.x or v4.x.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I know. I'm fine with these not being backported or only being partially backported.

}
// Mark as aborting so we can avoid sending queued request data
// This is used as a truthy flag elsewhere. The use of Date.now is for
Expand All @@ -329,8 +321,8 @@ ClientRequest.prototype.abort = function abort() {
};


function emitAbortNT(self) {
self.emit('abort');
function emitAbortNT() {
this.emit('abort');
}


Expand Down Expand Up @@ -682,26 +674,25 @@ function _deferToConnect(method, arguments_, cb) {
// calls that happen either now (when a socket is assigned) or
// in the future (when a socket gets assigned out of the pool and is
// eventually writable).
var self = this;

function callSocketMethod() {
const callSocketMethod = () => {
if (method)
self.socket[method].apply(self.socket, arguments_);
this.socket[method].apply(this.socket, arguments_);

if (typeof cb === 'function')
cb();
}
};

var onSocket = function onSocket() {
if (self.socket.writable) {
const onSocket = () => {
if (this.socket.writable) {
callSocketMethod();
} else {
self.socket.once('connect', callSocketMethod);
this.socket.once('connect', callSocketMethod);
}
};

if (!self.socket) {
self.once('socket', onSocket);
if (!this.socket) {
this.once('socket', onSocket);
} else {
onSocket();
}
Expand All @@ -710,10 +701,7 @@ function _deferToConnect(method, arguments_, cb) {
ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
if (callback) this.once('timeout', callback);

var self = this;
function emitTimeout() {
self.emit('timeout');
}
const emitTimeout = () => this.emit('timeout');

if (this.socket && this.socket.writable) {
if (this.timeoutCb)
Expand Down Expand Up @@ -752,3 +740,7 @@ ClientRequest.prototype.setSocketKeepAlive =
ClientRequest.prototype.clearTimeout = function clearTimeout(cb) {
this.setTimeout(0, cb);
};

module.exports = {
ClientRequest
};
24 changes: 13 additions & 11 deletions lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ const readStart = incoming.readStart;
const readStop = incoming.readStop;

const debug = require('util').debuglog('http');
exports.debug = debug;

exports.CRLF = '\r\n';
exports.chunkExpression = /(?:^|\W)chunked(?:$|\W)/i;
exports.continueExpression = /(?:^|\W)100-continue(?:$|\W)/i;
exports.methods = methods;

const kOnHeaders = HTTPParser.kOnHeaders | 0;
const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0;
Expand Down Expand Up @@ -194,7 +188,6 @@ var parsers = new FreeList('parsers', 1000, function() {

return parser;
});
exports.parsers = parsers;


// Free the parser and also break any links that it
Expand Down Expand Up @@ -227,7 +220,6 @@ function freeParser(parser, req, socket) {
socket.parser = null;
}
}
exports.freeParser = freeParser;


function ondrain() {
Expand All @@ -239,7 +231,6 @@ function httpSocketSetup(socket) {
socket.removeListener('drain', ondrain);
socket.on('drain', ondrain);
}
exports.httpSocketSetup = httpSocketSetup;

/**
* Verifies that the given val is a valid HTTP token
Expand Down Expand Up @@ -306,7 +297,6 @@ function checkIsHttpToken(val) {
}
return true;
}
exports._checkIsHttpToken = checkIsHttpToken;

/**
* True if val contains an invalid field-vchar
Expand Down Expand Up @@ -360,4 +350,16 @@ function checkInvalidHeaderChar(val) {
}
return false;
}
exports._checkInvalidHeaderChar = checkInvalidHeaderChar;

module.exports = {
_checkInvalidHeaderChar: checkInvalidHeaderChar,
_checkIsHttpToken: checkIsHttpToken,
chunkExpression: /(?:^|\W)chunked(?:$|\W)/i,
continueExpression: /(?:^|\W)100-continue(?:$|\W)/i,
CRLF: '\r\n',
debug,
freeParser,
httpSocketSetup,
methods,
parsers
};
Loading