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

fix(gateway): handle streaming errors, IPIP 332 #9333

Closed
wants to merge 8 commits into from
Closed
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
23 changes: 23 additions & 0 deletions core/corehttp/gateway_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -1104,3 +1104,26 @@ func (i *gatewayHandler) setCommonHeaders(w http.ResponseWriter, r *http.Request

return nil
}

// abortConn forcefully closes an HTTP connection, leading to a network error on the
// client side. This is a way of showing the client that there was an error while streaming
// the contents since there are no good ways of showing an error during a stream.
func abortConn(w http.ResponseWriter) {
hj, ok := w.(http.Hijacker)
if !ok {
// If we could not Hijack the connection (such as in HTTP/2.x) connections, we
// panic using http.ErrAbortHandler, which aborts the response.
// https://pkg.go.dev/net/http#ErrAbortHandler
panic(http.ErrAbortHandler)
}

conn, _, err := hj.Hijack()
if err != nil {
panic(http.ErrAbortHandler)
}

err = conn.Close()
if err != nil {
panic(http.ErrAbortHandler)
}
}
7 changes: 6 additions & 1 deletion core/corehttp/gateway_handler_block.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,12 @@ func (i *gatewayHandler) serveRawBlock(ctx context.Context, w http.ResponseWrite

// ServeContent will take care of
// If-None-Match+Etag, Content-Length and range requests
_, dataSent, _ := ServeContent(w, r, name, modtime, content)
_, dataSent, err := ServeContent(w, r, name, modtime, content)

if err != nil {
abortConn(w)
return
}

if dataSent {
// Update metrics
Expand Down
6 changes: 1 addition & 5 deletions core/corehttp/gateway_handler_car.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,7 @@ func (i *gatewayHandler) serveCAR(ctx context.Context, w http.ResponseWriter, r
car := gocar.NewSelectiveCar(ctx, store, []gocar.Dag{dag}, gocar.TraverseLinksOnlyOnce())

if err := car.Write(w); err != nil {
// We return error as a trailer, however it is not something browsers can access
// (https://github.com/mdn/browser-compat-data/issues/14703)
// Due to this, we suggest client always verify that
// the received CAR stream response is matching requested DAG selector
w.Header().Set("X-Stream-Error", err.Error())
abortConn(w)
return
}

Expand Down
10 changes: 1 addition & 9 deletions core/corehttp/gateway_handler_tar.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,6 @@ func (i *gatewayHandler) serveTAR(ctx context.Context, w http.ResponseWriter, r

// The TAR has a top-level directory (or file) named by the CID.
if err := tarw.WriteFile(file, rootCid.String()); err != nil {
w.Header().Set("X-Stream-Error", err.Error())
// Trailer headers do not work in web browsers
// (see https://github.com/mdn/browser-compat-data/issues/14703)
// and we have limited options around error handling in browser contexts.
// To improve UX/DX, we finish response stream with error message, allowing client to
// (1) detect error by having corrupted TAR
// (2) be able to reason what went wrong by instecting the tail of TAR stream
_, _ = w.Write([]byte(err.Error()))
return
abortConn(w)
}
}
2 changes: 1 addition & 1 deletion core/corehttp/gateway_handler_unixfs_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (i *gatewayHandler) serveDirectory(ctx context.Context, w http.ResponseWrit
logger.Debugw("request processed", "tplDataDNSLink", dnslink, "tplDataSize", size, "tplDataBackLink", backLink, "tplDataHash", hash)

if err := listingTemplate.Execute(w, tplData); err != nil {
internalWebError(w, err)
abortConn(w)
return
}

Expand Down
7 changes: 6 additions & 1 deletion core/corehttp/gateway_handler_unixfs_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,12 @@ func (i *gatewayHandler) serveFile(ctx context.Context, w http.ResponseWriter, r

// ServeContent will take care of
// If-None-Match+Etag, Content-Length and range requests
_, dataSent, _ := ServeContent(w, r, name, modtime, content)
_, dataSent, err := ServeContent(w, r, name, modtime, content)

if err != nil {
abortConn(w)
return
}

// Was response successful?
if dataSent {
Expand Down
3 changes: 1 addition & 2 deletions test/sharness/t0122-gateway-tar.sh
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ test_expect_success "Add CARs with relative paths to test with" '
'

test_expect_success "GET TAR with relative paths outside root fails" '
curl -o - "http://127.0.0.1:$GWAY_PORT/ipfs/$OUTSIDE_ROOT_CID?format=tar" > curl_output_filename &&
test_should_contain "relative UnixFS paths outside the root are now allowed" curl_output_filename
! curl "http://127.0.0.1:$GWAY_PORT/ipfs/$OUTSIDE_ROOT_CID?format=tar"
'

test_expect_success "GET TAR with relative paths inside root works" '
Expand Down