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

Fixed 500 reported for client disconnection #155

Merged
merged 6 commits into from
Aug 16, 2018
Merged

Fixed 500 reported for client disconnection #155

merged 6 commits into from
Aug 16, 2018

Conversation

coufalja
Copy link
Contributor

@coufalja coufalja commented Aug 6, 2018

What does this PR do?

Fix an issue with random 500s reported in Traefik metrics middleware. Issue: traefik/traefik#1926

Motivation

By default oxy middleware reports 500 for Context cancel error, which does not make much sense as in this case the 500 won't be received by the client and further HttpHandlers down the line (like Traefik metrics middleware) can go nuts because of it. I originally had this issue in Traefik so if it doesn't make sense to fix it in oxy I need to find a way how to fix it in Traefik only.

More

Fixes: traefik/traefik#1926

@coufalja
Copy link
Contributor Author

@juliens Any feedback on this? I am not sure if this is a right direction but I am really keen on getting some fix for those 500s into traefik 1.7 otherwise we need to run from our fork. The issue might be bigger for us as most of our clients access our servers over unreliable networks where disconnections are not uncommon.

@juliens
Copy link
Contributor

juliens commented Aug 13, 2018

I think, it's a good idea to catch the context.Canceled, but like this, in the access logs we got a 200, and I'm not sure it's a good idea to have 200 WDYT?

@coufalja
Copy link
Contributor Author

coufalja commented Aug 13, 2018

Hmm true, I would be for not outputting this request into access log at all. I think this is how apache/nginx access log handles the situation as well (while putting the line just into error log). TBH I am not sure how to achieve this in Traefik architecture. The second option would be to use some custom code, e.g. 599/499 or something. In that case, it would end up in both metrics and access log but it could be distinguished from regular 500

@juliens
Copy link
Contributor

juliens commented Aug 14, 2018

I prefer a solution with a specific status code, but I think this is more a 4x than a 5x.

@coufalja
Copy link
Contributor Author

coufalja commented Aug 15, 2018

Thanks, I have picked 499 the same as nginx does.
https://httpstatuses.com/499

Copy link
Contributor

@juliens juliens left a comment

Choose a reason for hiding this comment

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

It's better like this ;) thx
Just one little thing

utils/handler.go Outdated
statusText = http.StatusText(statusCode)
} else if err == context.Canceled {
statusCode = StatusClientClosedRequest
statusText = StatusClientClosedRequestText
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about create

func statusText(statusCode int) string {
    if statusCode == StatusClientClosedRequest {
        return StatusClientClosedRequestText
    }
    return http.StatusText(statusCode)
}

and use this new func in the Write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep good idea

Copy link
Contributor

@juliens juliens left a comment

Choose a reason for hiding this comment

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

👏 LGTM

@emilevauge emilevauge merged commit 885e42f into vulcand:master Aug 16, 2018
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.

Random 500 Error returned by Traefik
3 participants