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

Send errors instead of 100 Continue... #377

Closed
wants to merge 1 commit into from

Conversation

hamishforbes
Copy link
Contributor

... when requests have the Expect: 100-continue header.

Go's HTTP server automatically returns 100 Continue if the Expect header is there, unless you use the http.Error() method.
Also handles the content length and body automagically.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Very nice work, thanks! Got some feedback for you.

@@ -915,15 +915,8 @@ func (handler *UnroutedHandler) sendError(w http.ResponseWriter, r *http.Request
statusErr = NewHTTPError(err, http.StatusInternalServerError)
}

reason := append(statusErr.Body(), '\n')
if r.Method == "HEAD" {
reason = nil
Copy link
Member

Choose a reason for hiding this comment

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

Do you know how http.Error handles HEAD requests? Does it include the body or not?

if r.Method == "HEAD" {
reason = nil
}

w.Header().Set("Content-Type", "text/plain; charset=utf-8")
Copy link
Member

Choose a reason for hiding this comment

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

The http.Error method automatically sets the same Content-Type header, so could remove this here. See https://golang.org/src/net/http/server.go?s=62104:62156#L2011

@Acconut
Copy link
Member

Acconut commented Apr 23, 2020

Also, please have a look at the failing tests: https://github.com/tus/tusd/runs/604523101

@hamishforbes
Copy link
Contributor Author

I actually can't reproduce the problem now, I think I must have been testing through nginx in our dev env and getting tricked by their hardcoded 100-continue support!

http.Error() does not handle HEAD requests so this change in it's current state is a regression.
I'll just close.

If you have a chance though, could you please take a look over my other 2 PRs? Would be great to get those upstreamed and not have to maintain a fork!

@Acconut
Copy link
Member

Acconut commented May 1, 2020

👍 Thanks a lot for your efforts! I will have a look at your other PRs as soon as possible.

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.

2 participants