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

Handle store backend errors #325

Merged
merged 4 commits into from
Jul 19, 2021
Merged

Conversation

dtaniwaki
Copy link
Contributor

I got the following error with https://github.com/globocom/configurable-http-proxy-redis-backend when I hit a URL like http://chp.example.com/%25. Furthermore, the client didn't get a response forever.

06:52:05.473 [ConfigProxy] error: Error in handler for GET /%25: URIError: URI malformed
    at decodeURI (<anonymous>)
    at module.exports (/usr/local/lib/node_modules/configurable-http-proxy-redis-backend/node_modules/normalize-url/index.js:87:21)
    at ConfigurableProxyRedisStorage.cleanPath (/usr/local/lib/node_modules/configurable-http-proxy-redis-backend/lib/index.js:109:39)
    at ConfigurableProxyRedisStorage.getTarget (/usr/local/lib/node_modules/configurable-http-proxy-redis-backend/lib/index.js:189:42)
    at ConfigurableProxy.targetForReq (/usr/local/lib/node_modules/configurable-http-proxy/lib/configproxy.js:387:25)
    at ConfigurableProxy.handleProxy (/usr/local/lib/node_modules/configurable-http-proxy/lib/configproxy.js:531:17)
    at ConfigurableProxy.handleProxyWeb (/usr/local/lib/node_modules/configurable-http-proxy/lib/configproxy.js:610:17)
    at Server.<anonymous> (/usr/local/lib/node_modules/configurable-http-proxy/lib/configproxy.js:198:27)
    at Server.emit (events.js:376:20)
    at parserOnIncoming (_http_server.js:896:12)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:126:17)

Although this error should be taken care of by the store backend, CHP should make a response at least.

@dtaniwaki dtaniwaki force-pushed the handle-backend-error branch from 097b1c4 to c6f7cea Compare June 24, 2021 09:39
@consideRatio
Copy link
Member

@minrk I find it very hard to review in CHP =/ Can you help out on this?

to ensure it returns a promise, no matter where errors may occur
target: route.data.target,
};
}
return new Promise((resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

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

Noting for review:

wrapping this in a promise ensures that targetForReq always returns a Promise, even if routes.getTarget raises directly (as opposed to returning a failing promise).

Comment on lines +602 to +605
.catch(function (e) {
if (res.finished) throw e;
that.handleProxyError(500, kind, req, res, e);
});
Copy link
Member

Choose a reason for hiding this comment

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

These lines are the only changes in this function: adding the catch. prettier chooses a different indentation when there's both a .then and a .catch, which makes this PR look bigger than it is.

@minrk minrk merged commit fa3a212 into jupyterhub:main Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants