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

HTTP status 499 instead of 502's for 'Context canceled' 'errors' #2297

Merged
merged 10 commits into from Nov 16, 2018

Conversation

4 participants
@Ranger-X
Contributor

Ranger-X commented Sep 19, 2018

1. What does this change do, exactly?

Replaces (not an) HTTP error code 502 'Context canceled' with the 499 'Context canceled' as suggested in https://stackoverflow.com/questions/46234679/what-is-the-correct-http-status-code-for-a-cancelled-request

2. Please link to the relevant issues.

#1925
#1828
https://caddy.community/t/error-context-canceled/2034/9
and others

3. Which documentation changes (if any) need to be made because of this PR?

None

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later

Ranger-X and others added some commits Aug 8, 2018

Add an 499 HTTP status code on user's cancel request as NGINX doing (…
…instead of 502 Bad Gateway status with 'Context canceled' message)
@francislavoie

This comment has been minimized.

Collaborator

francislavoie commented Oct 30, 2018

@mholt This looks pretty safe, IMO just needs a note in the proxy docs about this behaviour. See the TODO added, where should that const go, if at all?

@mholt

Alright, this is a good idea I suppose. A 4xx error makes more sense because the client caused the error.

Show resolved Hide resolved caddyhttp/proxy/proxy.go Outdated

Ranger-X and others added some commits Oct 30, 2018

@mholt

mholt approved these changes Nov 16, 2018

@mholt

This comment has been minimized.

Owner

mholt commented Nov 16, 2018

Appveyor failure is unrelated. Just waiting for the CLA to be signed / verified.

@Ranger-X

This comment has been minimized.

Contributor

Ranger-X commented Nov 16, 2018

I think I have already signed CLA. @mholt could you to recheck PRs? (cla-assistant/cla-assistant#124)

@mholt mholt merged commit 05d0b21 into mholt:master Nov 16, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@mholt

This comment has been minimized.

Owner

mholt commented Nov 16, 2018

Got it, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment