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.

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/blockchain_process): swallow errors, revise streams #1181

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

michaelsbradleyjr
Copy link
Contributor

For reasons unknown, ECONNRESET errors on websocket connections to embark's blockchain proxy are not automatically handled on Windows as they are on macOS and Linux (or those errors aren't happening on those platforms, it's difficult to determine). Explicitly swallow such errors so the blockchain process doesn't crash. Prior to this PR, the crash-behavior can be reproduced on Windows by running embark blockchain and embark run in separate terminals and quitting embark run while embark blockchain is still running.

Consistently use the simples package's WsParser to process websocket traffic instead of using WsParser for requests and the ws package's Websocket.Receiver for responses.

Consistently use pump to connect parser streams instead of using pump in some places and chain in others. Drop use of cloneable (and the package dependency) since it was used previously in hopes it would fix the errors, but it's unnecessary and didn't fix them.

@michaelsbradleyjr michaelsbradleyjr requested a review from a team December 11, 2018 23:59
@michaelsbradleyjr
Copy link
Contributor Author

This PR works together with #1166 to prevent hangs and crashes when running / stopping embark processes.

setImmediate(destroy);
};
req.on('error', swallowECONNRESET);
socket.on('error', swallowECONNRESET);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this cause problem for other types of errors?

Copy link
Contributor Author

@michaelsbradleyjr michaelsbradleyjr Dec 12, 2018

Choose a reason for hiding this comment

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

Looks like I made a mistake last night when rebasing and used an older commit... dangers of develping / testing on 3 different computers at the same time. I'm going to sort that out now.

Here's what that part will look like:

if (ws) {
  server.on('upgrade', (msg, socket, head) => {
    const swallowError = (err) => {
      console.error(`Proxy: Network error '${err.message}'`);
    };
    socket.on('error', swallowError);
    proxy.upgrade(msg, socket, head);
  });
}

I checked yesterday and here's what the docs say about net.Socket's 'error' event:

Emitted when an error occurs. The 'close' event will be called directly following this event.

Since the socket will be closed, no need to destroy it manually and there's not much more we can / should do at that point other than report with console.error().

Also, the variable I had named req previously is actually an http.IncomingMessage instance. It lacks an 'error' event and its destroy() method just destroys the underlying socket (msg.socket, alias msg.connection), which is identity with the object in the socket variable, so we don't need to do anything with it.

For reasons unknown, `ECONNRESET` errors on websocket connections to embark's
blockchain proxy are not automatically handled on Windows as they are on macOS
and Linux (or those errors aren't happening on those platforms, it's difficult
to determine). Explicitly swallow such errors so the blockchain process doesn't
crash. Prior to this PR, the crash-behavior can be reproduced on Windows by
running `embark blockchain` and `embark run` in separate terminals and quitting
`embark run` while `embark blockchain` is still running.

Consistently use the `simples` package's `WsParser` to process websocket
traffic instead of using `WsParser` for requests and the `ws` package's
`Websocket.Receiver` for responses.

Consistently use `pump` to connect parser streams instead of using `pump` in
some places and `chain` in others. Drop use of `cloneable` (and the package
dependency) since it was used previously in hopes it would fix the errors, but
it's unnecessary and didn't fix them.
@michaelsbradleyjr michaelsbradleyjr force-pushed the refactor/blockchain-proxy-round-2 branch from f1ae969 to 5ec9d0c Compare December 12, 2018 16:25
@0x-r4bbit 0x-r4bbit merged commit e738efe into master Dec 13, 2018
@0x-r4bbit 0x-r4bbit deleted the refactor/blockchain-proxy-round-2 branch December 13, 2018 15:28
michaelsbradleyjr added a commit that referenced this pull request Dec 14, 2018
The problems described in embark PR #1166 can be resolved by implementing the
blockchain proxy with `http-proxy` directly instead of using `express` together
with `http-proxy-middleware`. The ultimate cause of the buggy behavior (the
"stuck sockets" problems described in #1166) is unknown.

The need to swallow some errors as described in embark PR #1181 is also
eliminated by dropping `http-proxy-middleware` and `express`.
michaelsbradleyjr added a commit that referenced this pull request Dec 14, 2018
The problems described in embark PR #1166 can be resolved by implementing the
blockchain proxy with `http-proxy` directly instead of using `express` together
with `http-proxy-middleware`. The ultimate cause of the buggy behavior (the
"stuck sockets" problems described in #1166) is unknown.

The need to swallow some errors as described in embark PR #1181 is also
eliminated by dropping `http-proxy-middleware` and `express`.
michaelsbradleyjr added a commit that referenced this pull request Dec 14, 2018
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 additional
`'error'` events that might fire after the first `'error'` event, but do report
them.
michaelsbradleyjr added a commit that referenced this pull request Dec 14, 2018
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 added a commit that referenced this pull request Dec 17, 2018
The problems described in embark PR #1166 can be resolved by implementing the
blockchain proxy with `http-proxy` directly instead of using `express` together
with `http-proxy-middleware`. The ultimate cause of the buggy behavior (the
"stuck sockets" problems described in #1166) is unknown.

The need to swallow some errors as described in embark PR #1181 is also
eliminated by dropping `http-proxy-middleware` and `express`.
michaelsbradleyjr added a commit that referenced this pull request Dec 18, 2018
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 added a commit that referenced this pull request Dec 18, 2018
The problems described in embark PR #1166 can be resolved by implementing the
blockchain proxy with `http-proxy` directly instead of using `express` together
with `http-proxy-middleware`. The ultimate cause of the buggy behavior (the
"stuck sockets" problems described in #1166) is unknown.

The need to swallow some errors as described in embark PR #1181 is also
eliminated by dropping `http-proxy-middleware` and `express`.
michaelsbradleyjr added a commit that referenced this pull request Dec 18, 2018
The problems described in embark PR #1166 can be resolved by implementing the
blockchain proxy with `http-proxy` directly instead of using `express` together
with `http-proxy-middleware`. The ultimate cause of the buggy behavior (the
"stuck sockets" problems described in #1166) is unknown.

The need to swallow some errors as described in embark PR #1181 is also
eliminated by dropping `http-proxy-middleware` and `express`.
michaelsbradleyjr added a commit that referenced this pull request Dec 18, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants