refactor(@embark/blockchain_process): remove http-proxy-middleware #1195
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrainville
approved these changes
Dec 14, 2018
michaelsbradleyjr
force-pushed
the
refactor/blockchain-proxy-round-3
branch
from
bdfb14f to
299c3fe
Compare
This was referenced Dec 14, 2018
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.
0x-r4bbit
suggested changes
Dec 17, 2018
| }; | ||
|
|
||
| Blockchain.prototype.setupProxy = async function () { | ||
| const AccountParser = require('../../utils/accountParser'); |
Contributor
There was a problem hiding this comment.
Can we move this require statement out of the function? Ideally we refactor this straight to import * as AccountParser ..
Actually it'd be nicer if we could do import { AccountParser } from '... but we'll have to refactor the exports of AccountParser for that.
| case METHODS_TO_MODIFY.accounts: | ||
| body.result = body.result.concat(accounts); | ||
| break; | ||
| default: |
Contributor
There was a problem hiding this comment.
I think default should return something here.
Collaborator
There was a problem hiding this comment.
Well, it should not modify or change the behavior in case of default. I used a switch because it's easier that way to add more cases than an ìf`
| head.push(`${key}: ${value}`); | ||
| return head; | ||
| } | ||
| for (let i = 0; i < value.length; i++) { |
Contributor
There was a problem hiding this comment.
Can we go with value.forEach here?
| server.emit('proxyReqWs', proxyReq, req, socket, options, head); | ||
| } | ||
|
|
||
| // Error Handler |
Contributor
There was a problem hiding this comment.
This comment is unnecessary
| const http = require('http'); | ||
| const https = require('https'); | ||
| const httpProxyWsIncoming = require('http-proxy/lib/http-proxy/passes/ws-incoming'); | ||
| const common = require('http-proxy/lib/http-proxy/common'); |
Contributor
There was a problem hiding this comment.
Since this is a newly introduced file, it'd be great if we could go with import statements right away
| const CRLF = '\r\n'; | ||
|
|
||
| httpProxyWsIncoming.stream = (req, socket, options, head, server, cb) => { | ||
| const createHttpHeader = function(line, headers) { |
Contributor
There was a problem hiding this comment.
I believe this function can be defined outside of stream no?
Also, if we go with function initialization instead of declaration, can we use arrow functions instead?
Contributor
Author
There was a problem hiding this comment.
httpProxyOverride.js was copied from an unmerged PR for the http-proxy package:
http-party/node-http-proxy#1301
We're using it to patch http-proxy with support for transform streams.
Some light cleanup was done to the formatting of the copied code, but it wasn't otherwise modified. We certainly can introduce additional changes, but more time will need to be spent studying the source to make sure we don't break it.
| // The pipe below will end proxySocket if socket closes cleanly, but not | ||
| // if it errors (eg, vanishes from the net and starts returning | ||
| // EHOSTUNREACH). We need to do that explicitly. | ||
| socket.on('error', function () { |
Contributor
There was a problem hiding this comment.
Also here, for all the anonymous functions above, I think this would be nicer with arrow functions.
| const testLogger = new TestLogger({}); | ||
| }; | ||
| isHexStrictStub = sinon.stub(Web3.utils, 'isHexStrict').returns(true); | ||
| // Web3.utils.isHexStrict = sinon.stub().returns(true); |
Contributor
There was a problem hiding this comment.
There's still a comment left here.
| }); | ||
|
|
||
| if (ws) { | ||
| server.on('upgrade', function (msg, socket, head) { |
Contributor
There was a problem hiding this comment.
Can we go with arrow function here too? :)
|
|
||
| /*eslint complexity: ["error", 30]*/ | ||
| static getAccount(accountConfig, web3, logger = console, nodeAccounts) { | ||
| const {utils} = require('web3'); |
Contributor
There was a problem hiding this comment.
Let's move this require statement out of this function.
Collaborator
|
FYI, this PR now contains #1172 |
Collaborator
|
@PascalPrecht Like Michael mentioned, the file httpProxyOverride.js is a patch from a PR that was never merged in |
michaelsbradleyjr
force-pushed
the
refactor/blockchain-proxy-round-3
branch
2 times, most recently
from
f867cc1 to
291146d
Compare
Contributor
Author
|
Rebased against master, mainly accounting for #1156. |
0x-r4bbit
approved these changes
Dec 18, 2018
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.
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
force-pushed
the
refactor/blockchain-proxy-round-3
branch
from
291146d to
bd6e4a6
Compare
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problems described in embark PR #1166 can be resolved by implementing the blockchain proxy with
http-proxydirectly instead of usingexpresstogether withhttp-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-middlewareandexpress.