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

refactor(@embark/utils): use a 'ws' websocket client in pingEndpoint #1196

Merged
merged 1 commit into from
Dec 18, 2018
Merged

refactor(@embark/utils): use a 'ws' websocket client in pingEndpoint #1196

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 52 additions & 35 deletions src/lib/utils/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,44 +93,61 @@ function getJson(url, cb) {
}

function pingEndpoint(host, port, type, protocol, origin, callback) {
const options = {
protocolVersion: 13,
perMessageDeflate: true,
origin: origin,
host: host,
port: port
// remove any extra information from a host string, e.g. port, path, query
const _host = require('url').parse(
// url.parse() expects a protocol segment else it won't parse as desired
host.slice(0, 4) === 'http' ? host : `${protocol}://${host}`
).hostname;

let closed = false;
const close = (req, closeMethod) => {
if (!closed) {
closed = true;
req[closeMethod]();
}
};
if (type === 'ws') {
options.headers = {
'Sec-WebSocket-Version': 13,
Connection: 'Upgrade',
Upgrade: 'websocket',
'Sec-WebSocket-Extensions': 'permessage-deflate; client_max_window_bits',
Origin: origin
};
}
let req;
// remove trailing api key from infura, ie rinkeby.infura.io/nmY8WtT4QfEwz2S7wTbl
if (options.host.indexOf('/') > -1) {
options.host = options.host.split('/')[0];
}
if (protocol === 'https') {
req = require('https').get(options);
} else {
req = require('http').get(options);
}

req.on('error', (err) => {
callback(err);
});
const handleEvent = (req, closeMethod, ...args) => {
close(req, closeMethod);
setImmediate(() => { callback(...args); });
};

req.on('response', (_response) => {
callback();
});
const handleError = (req, closeMethod) => {
req.on('error', (err) => {
if (err.code !== 'ECONNREFUSED') {
console.error(
`Ping: Network error` +
(err.message ? ` '${err.message}'` : '') +
(err.code ? ` (code: ${err.code})` : '')
);
}
// when closed additional error events will not callback
if (!closed) { handleEvent(req, closeMethod, err); }
});
};

req.on('upgrade', (_res, _socket, _head) => {
callback();
});
const handleSuccess = (req, closeMethod, event) => {
req.once(event, () => { handleEvent(req, closeMethod); });
};

const handleRequest = (req, closeMethod, event) => {
handleError(req, closeMethod);
handleSuccess(req, closeMethod, event);
};

if (type === 'ws') {
const req = new (require('ws'))(
`${protocol === 'https' ? 'wss' : 'ws'}://${_host}:${port}/`,
origin ? {origin} : {}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this require statement out of this function and put it at the top of the file?

Copy link
Contributor Author

@michaelsbradleyjr michaelsbradleyjr Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about instead a quick follow-up PR (I'll do it) that lifts all of the requires in utils.js to top-level imports?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't do it!!
There are some requires that are super heavy that are required just by one or two methods that are then used by some processes and it slows those processes a lot.

The real fix would be to split the utils file in multiple files and then we can put the requires at the top, but they would be used by all or most of the functions.

Copy link
Contributor Author

@michaelsbradleyjr michaelsbradleyjr Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the heads-up! Another options would be to make light static imports top-level; for the ones we know to be heavy we could use inline dynamic import() (our babel config already supports it). Dynamic import is async (returns a promise) whereas require() is synchronous, so might require some refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some requires that are super heavy that are required just by one or two methods that are then used by some processes and it slows those processes a lot.

@jrainville do we know which (or at least some) that fall under this category? I'm actually surprised that this is a problem for process performance..

The real fix would be to split the utils file in multiple files and then we can put the requires at the top, but they would be used by all or most of the functions.

Yea I don't really think this can be considered a fix. I'd rather figure out how loading (bigger) node modules can slow down processes.

Maybe we can get some benchmarks for some reproducible scenarios.

for the ones we know to be heavy we could use inline dynamic import() (our babel config already supports it).

Happy to explore the dynamic import route at some point. But "checking" every imported module whether or not it's too "heavy" doesn't really sound like the right move to me.

I hope we can take some time in the near future getting to the bottom of this and figuring out how what affects what.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anything that requires web is super heavy.
An example of a process that could be slowed in the pipeline, it might need to use a small utils functions, but then it would need to require every package, include web3.

handleRequest(req, 'close', 'open');
} else {
const headers = origin ? {Origin: origin} : {};
const req = (protocol === 'https' ? require('https') : require('http')).get(
{headers, host: _host, port}
);
handleRequest(req, 'abort', 'response');
}
}

function runCmd(cmd, options, callback) {
Expand Down Expand Up @@ -557,7 +574,7 @@ function isNotFolder(node){
}

function byName(a, b) {
return a.name.localeCompare(b.name);
return a.name.localeCompare(b.name);
}

function fileTreeSort(nodes){
Expand Down