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

Jump to bottom

Handle proxy errors #477

Merged
merged 1 commit into from Apr 15, 2019
Merged

Handle proxy errors #477

merged 1 commit into from Apr 15, 2019

Conversation

nLight
Copy link
Contributor

@nLight nLight commented Nov 7, 2018

http-proxy does not handle errors by default and would just throw crashing the server. This PR adds error handling with rudimental logging.

@nLight
Copy link
Contributor Author

nLight commented Nov 7, 2018

I couldn't really figure out how to test this with the testing setup. Any hint would be appreciated.

@nLight nLight mentioned this pull request Nov 7, 2018
Closed
@nLight
Copy link
Contributor Author

nLight commented Nov 22, 2018

@BigBlueHat anything I could do to move that one forward?

@patrikbeno
Copy link

I couldn't really figure out how to test this with the testing setup. Any hint would be appreciated.

In my case, http-server crashes if the proxy is not available, so I guess you could simply point proxy to the free localhost port and then hit http-server with non-existent URL.

It will fail and abort like this:

index.js:120
    throw err;
    ^

Error: connect ECONNREFUSED 127.0.0.1:8080
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1113:14)

@nLight
Copy link
Contributor Author

nLight commented Dec 12, 2018

@BigBlueHat any chance looking into this?

@nLight
Copy link
Contributor Author

nLight commented Dec 12, 2018

@patrikbeno figuring out a test case is easy, sure. What I would like to get help with is fitting this test in the testing env of the project.

@nLight
Copy link
Contributor Author

nLight commented Dec 12, 2018

@indexzero ping?

BigBlueHat
BigBlueHat reviewed Dec 12, 2018
View reviewed changes
lib/http-server.js Outdated
@@ -107,6 +107,14 @@ function HttpServer(options) {

if (typeof options.proxy === 'string') {
var proxy = httpProxy.createProxyServer({});
proxy.on('error', function (err) {
if (options.logFn) { // If there's a log function for requests we can log
Copy link
Member

Choose a reason for hiding this comment

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

If there's a log function set, the code should use that and not just console.info. See https://github.com/indexzero/http-server/blob/94329688b5bf6c058a9f2c24739bd628b7345dce/lib/http-server.js#L62-L64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I fake req and res arguments in this case? I don't have them in this context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping

Copy link
Member

Choose a reason for hiding this comment

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

Move the on('error') code into the before.push https://github.com/indexzero/http-server/blob/f65f1a7154456f823261c4f8e90b35eff66eb23a/lib/http-server.js#L118 where the proxy is added into the mix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a cleaner way to do it. Force-pushed, please review 🙇

@nLight
Copy link
Contributor Author

nLight commented Jan 21, 2019

Any progress? @BigBlueHat crashing proxies is not cool

@nLight
Copy link
Contributor Author

nLight commented Jan 30, 2019

Just a friendly reminder

@nLight
Copy link
Contributor Author

nLight commented Feb 6, 2019

Just another friendly reminder @BigBlueHat

@thornjad thornjad added the minor version non-breaking, non-trivial change label Feb 6, 2019
@nLight
Copy link
Contributor Author

nLight commented Feb 6, 2019

Thank you 🙇

@thornjad thornjad added this to the v0.12.0 milestone Feb 28, 2019
@thornjad thornjad mentioned this pull request Apr 15, 2019
Closed
@thornjad thornjad merged commit 0b69655 into http-party:master Apr 15, 2019
thornjad added a commit that referenced this pull request Apr 15, 2019
@thornjad
Merge pull request #477 from dcos-labs/add-prox...
e703c5b 
...y-error-handling

Handle proxy errors

Fixes #193
@thornjad thornjad mentioned this pull request Apr 15, 2019
Closed
@nLight
Copy link
Contributor Author

nLight commented Apr 15, 2019

🎉

@nLight nLight deleted the add-proxy-error-handling branch April 15, 2019 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BigBlueHat BigBlueHat BigBlueHat left review comments

@thornjad thornjad thornjad approved these changes

Assignees
No one assigned
Labels
minor version non-breaking, non-trivial change
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nLight @patrikbeno @BigBlueHat @thornjad