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

caddyhttp: Set http.error.message to the HandlerError message #4971

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

francislavoie
Copy link
Member

Fix #4969

@francislavoie francislavoie added the bug 🐞 Something isn't working label Aug 21, 2022
@francislavoie francislavoie added this to the v2.5.3 milestone Aug 21, 2022
@mholt
Copy link
Member

mholt commented Aug 23, 2022

Thanks! Just to make sure I understand, does this mean that {http.error} is the generic error message if no custom message is set, and that {http.error.message} is always only the custom message?

@francislavoie
Copy link
Member Author

For example:

:8883 {
    error "Custom Error Message" 503

    handle_errors {
        respond "
Message: {http.error.message}
Error: {http.error}
Status: {http.error.status_code}
Text: {http.error.status_text}
Trace: {http.error.trace}
"
    }
}

Will write out:

Message: Custom Error Message
Error: {id=1bdsihwsh} caddyhttp.StaticError.ServeHTTP (staticerror.go:110): HTTP 503: Custom Error Message
Status: 503
Text: Service Unavailable
Trace: caddyhttp.StaticError.ServeHTTP (staticerror.go:110)

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Ah that's pretty good. LGTM! Thanks for the patch. Not sure how I forgot this... heh.

@mholt mholt changed the title caddyhttp: Set http.error.message with the HandlerError message caddyhttp: Set http.error.message to the HandlerError message Aug 23, 2022
@mholt mholt merged commit 72541f1 into master Aug 23, 2022
@mholt mholt deleted the handler-error-message branch August 23, 2022 05:31
@francislavoie
Copy link
Member Author

Yeah it's kinda wild, we've always documented it but it never worked 🤣

@bones-was-here
Copy link

Thanks for implementing this, but there seems to be a regression. When file_server generates a 404, Caddy simply terminates the connection without returning any status code or body. It doesn't log anything to stdout/stderr but I'm guessing it doesn't like handlerErr.Err.Error() being called when no message is set. It returns an error normally when the message is set by the error directive.

@francislavoie
Copy link
Member Author

Please provide your full config and a reproduce case. I don't want to make assumptions like we had to do in the previous thread.

I find it very unlikely that this PR would have caused any kind of regression. It only sets a variable, that's it.

@francislavoie
Copy link
Member Author

Alright yep I see it. handlerErr.Err is nil in that case. Whoops.

Making a followup PR.

francislavoie added a commit that referenced this pull request Aug 23, 2022
Followup to #4971, need to check that there actually is a wrapped error before trying to grab its message.
@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom error message string is not output anywhere
3 participants