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

handler: Fix error with RUFH upload ending too soon #1098

Merged
merged 2 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions pkg/handler/body_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,28 @@ func (r *bodyReader) Read(b []byte) (int, error) {

}
if err != nil {
// We can ignore some of these errors:
// - io.EOF means that the request body was fully read
// - io.ErrBodyReadAfterClose means that the bodyReader closed the request body because the upload is
// is stopped or the server shuts down.
// Note: if an error occurs while reading the body, we must set `r.err` (either in here
// or somewhere else, such as in closeWithError). Otherwise, the PATCH handler might not know
// that an error occurred and assumes that a request was transferred succesfully even though
// it was interrupted. This leads to problems with the RUFH draft.

// io.EOF means that the request body was fully read and does not represent an error.
if err == io.EOF {
return n, io.EOF
}

// http.ErrBodyReadAfterClose means that the bodyReader closed the request body because the upload is
// is stopped or the server shuts down. In this case, the closeWithError method already
// set `r.err` and thus we don't overerwrite it here but just return.
if err == http.ErrBodyReadAfterClose {
return n, io.EOF
}

// All of the following errors can be understood as the input stream ending too soon:
// - io.ErrClosedPipe is returned in the package's unit test with io.Pipe()
// - io.UnexpectedEOF means that the client aborted the request.
// In all of those cases, we do not forward the error to the storage,
// but act like the body just ended naturally.
if err == io.EOF || err == io.ErrClosedPipe || err == http.ErrBodyReadAfterClose || err == io.ErrUnexpectedEOF {
return n, io.EOF
if err == io.ErrClosedPipe || err == io.ErrUnexpectedEOF {
err = ErrUnexpectedEOF
}

// Connection resets are not dropped silently, but responded to the client.
Expand Down
1 change: 1 addition & 0 deletions pkg/handler/unrouted_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ var (
ErrUploadInterrupted = NewError("ERR_UPLOAD_INTERRUPTED", "upload has been interrupted by another request for this upload resource", http.StatusBadRequest)
ErrServerShutdown = NewError("ERR_SERVER_SHUTDOWN", "request has been interrupted because the server is shutting down", http.StatusServiceUnavailable)
ErrOriginNotAllowed = NewError("ERR_ORIGIN_NOT_ALLOWED", "request origin is not allowed", http.StatusForbidden)
ErrUnexpectedEOF = NewError("ERR_UNEXPECTED_EOF", "server expected to receive more bytes", http.StatusBadRequest)

// These two responses are 500 for backwards compatability. Clients might receive a timeout response
// when the upload got interrupted. Most clients will not retry 4XX but only 5XX, so we responsd with 500 here.
Expand Down