refactor(@embark/utils): use a 'ws' websocket client in pingEndpoint #1196
Conversation
src/lib/utils/utils.js
Outdated
| host: host, | ||
| port: port | ||
| // remove trailing api key from infura, e.g. rinkeby.infura.io/nmY8WtT4QfEwz2S7wTbl | ||
| const _host = host.indexOf('/') > -1 ? host.split('/')[0] : host; |
There was a problem hiding this comment.
In other words, we're purely interested in the host, not the path. The comment here threw me off cause it made it sound like this API was designed for infura endpoints.
There was a problem hiding this comment.
This comment and code were carried over from the previous implementation, but it does need another look since host could mistakenly contain a a :[port] section and when the websocket connection is constructed it would be an invalid url. Thanks for raising this section to my attention!
There was a problem hiding this comment.
See: 5a16903
I made it more generic. Note that if a ping really should be tried with a path other than / (maybe with query parameters), then pingEndpoint would need to be revised to allow for an additional path parameter, or maybe an options parameter-object that could include path:.
| const req = new (require('ws'))( | ||
| `${protocol === 'https' ? 'wss' : 'ws'}://${_host}:${port}/`, | ||
| {origin} | ||
| ); |
There was a problem hiding this comment.
Can we move this require statement out of this function and put it at the top of the file?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR replaces #1166. The "stuck sockets" bug is addressed in #1195 so there is no longer a need to use timeouts. However a few aspects of the original PR are still useful, and lessons learned from #1166, #1181, and #1195 can be put to good use. Use a websocket client from the `ws` package when pinging websocket endpoints instead of manually building a request header. The `'upgrade'` event being listened for was never actually firing; and though a response was received for those pings, the response messages indicated problems with those requests. It seems cleaner to use a proper websocket client and callback success upon the `ws` client's `'open'` event. Abstract error and success handling across websocket and http pings. Report network errors other than `ECONNREFUSED`. Only `ECONNREFUSED` is expected, as that genuinely indicates an endpoint isn't accepting connections at the specified host and port. If other kinds of network errors are occurring, it will be helpful to have a visual indicator to prompt investigation. After success or the first error, cleanup the ping's request/connection immediately since we're not awaiting `'data'` events on an http request and we don't want to leave a websocket connection open. Don't callback any `'error'` events that might fire after the first `'error'` event or a success event, but do report them.
michaelsbradleyjr
force-pushed
the
refactor/pingEndpoint
branch
from
93e86d9 to
76bd74a
Compare
This PR replaces #1166. The "stuck sockets" bug is addressed in #1195 so there is no longer a need to use timeouts. However a few aspects of the original PR are still useful, and lessons learned from #1166, #1181, and #1195 can be put to good use.
Use a websocket client from the
wspackage when pinging websocket endpoints instead of manually building a request header. The'upgrade'event being listened for was never actually firing; and though a response was received for those pings, the response messages indicated problems with those requests. It seems cleaner to use a proper websocket client and callback success upon thewsclient's'open'event.Abstract error and success handling across websocket and http pings.
Report network errors other than
ECONNREFUSEDwithconsole.error(). OnlyECONNREFUSEDis expected, as that genuinely indicates an endpoint isn't accepting connections at the specified host and port. If other kinds of network errors are occurring, it will be helpful to have a visual indicator to prompt investigation.After success or the first error, cleanup the ping's request/connection immediately since we're not awaiting
'data'events on an http request and we don't want to leave a websocket connection open. Don't callback any'error'events that might fire after the first'error'event or a success event, but do report them.