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

Warn users with debug enabled that middleware errors are not handled #2374

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

apatruni
Copy link

Warn users with debug enabled that middleware errors are not handled

Users may not expect errors thrown in the middleware on the response path flow to be ignored if the response has been committed by a handler

@aldas
Copy link
Contributor

aldas commented Jan 2, 2023

I have somewhat mixed feeling about it. I understand and see cases when this is useful but same time it would cause potentially a lot of noise for cases when that early-return is totally OK.

There is

echo/response.go

Lines 54 to 58 in f36d566

func (r *Response) WriteHeader(code int) {
if r.Committed {
r.echo.Logger.Warn("response already committed")
return
}
and I know from my own experience that it can cause a lot of log lines.

Maybe we should document all methods on echo.Context what "commit" the Response with short block explaining what commiting means to error handling and for example why you should return errors from handler and not c.String(http.StatusBadRequest, "NOPE") sending response.

also. 3 lines above of

echo/echo.go

Line 391 in f36d566

if c.Response().Committed {
there is short explanation about "commited" response.

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.

None yet

2 participants