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.

Sign up for GitHub

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

IPIP-332: Streaming Error Handling on Web Gateways #332

Closed
wants to merge 11 commits into from
Closed

IPIP-332: Streaming Error Handling on Web Gateways #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8a175f6
ipip: error handling in gateways
hacdias Oct 12, 2022
a84fb63
ipip: cleanup streaming and should
hacdias Oct 12, 2022
4e73544
Update IPIP/0000-gateway-error-handling.md
hacdias Oct 12, 2022
8ccb87d
Merge branch 'main' into feat/error-handling-gateway
hacdias Nov 7, 2022
9480caa
add stream close details for http 1.1 and http 2.0
hacdias Nov 7, 2022
a20a665
Update http-gateways/PATH_GATEWAY.md
hacdias Nov 8, 2022
e7f6b54
Update http-gateways/PATH_GATEWAY.md
hacdias Nov 8, 2022
3e3b0fe
Merge branch 'main' into feat/error-handling-gateway
hacdias Nov 10, 2022
0fcdd02
ipip: add headers info
hacdias Nov 10, 2022
4dd1293
ipip: add header info
hacdias Nov 10, 2022
c0c6af7
ipip: update design
hacdias Nov 10, 2022
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
72 changes: 72 additions & 0 deletions IPIP/0332-gateway-error-handling.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# IPIP 0000: Streaming Error Handling in HTTP Gateways

- Start Date: 2022-10-12
- Related Issues:
- [ipfs/kubo/pull/9333](https://github.com/ipfs/kubo/pull/9333)
- [mdn/browser-compat-data/issues/14703](https://github.com/mdn/browser-compat-data/issues/14703)

## Summary

Ensure streaming error handling in web gateways is clear and consistent.

## Motivation

Web gateways provide different functionalities where users can download files.
The download of this files is streamed from the server to the client using HTTP.
However, there is no good way of presenting to the client an error that happens
during the stream.

For example, if during the download of a TAR file, the server detects some error
and is not able to continue, the user can get a valid, yet incomplete TAR. However,
the user will not know that the TAR is incomplete. By introducing consistent error
handling, the server attempts to notify the user.

## Detailed design

If the server encounters an error before streaming the contents to the client,
the server must fail with the respective `4xx` or `5xx` HTTP status code (no change).

If the server encounters an error while streaming the contents, the server must
force-close the HTTP stream to the user. This way, the user will receive a
network error, making it clear that the downloaded file is not valid.
In addition, the server must set an header `X-On-Error: reset` to indicate that
connection will be reset if an error occurs.

The client can opt-out of this behavior by sending a `X-Error-Behavior: trailer`.
In that case, the server will not force-close the HTTP stream. Instead, the server
will send the trailing header `X-Stream-Error` header with the error message.
Comment on lines +35 to +37
Copy link
Member

@lidel lidel Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I know this was opt-in, but as usual, reminding that Trailers can't be read by JS in web browsers: mdn/browser-compat-data#14703 (lesson learned from Kubo RPC: ipfs/js-ipfs#2519)
Gateway specs should never require things that are not supported in browsers, as this leads to the very problem types this IPIP aims to solve.


## Test fixtures

There are no relevant test fixures for this IPIP.

## Design rationale

Before starting to stream the body of the response, the server is able to set
an HTTP status code for the error. However, after the HTTP headers are set
and the body started being streamed, there are no clear ways in the HTTP
specification to show an error. Since the gateway is browser-first, it is
important to show an error and avoid users receiving an incomplete file.
Therefore, the server can force-close the HTTP stream, leading to a network
error. This tells the user that an error happened.

### User benefit

The user will know that an error happened while receiving the file. Otherwise,
the user might receive incomplete, but still valid, files that could be mistaken
but the real file.

### Compatibility

This RFC is backwards compatible.

### Alternatives

Using [`Trailer`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer) HTTP headers
was considered. However, trailer headers are [not supported in browsers](https://github.com/mdn/browser-compat-data/issues/14703).
In addition, even if trailer headers were supported in browsers, there is no clear
standard for which header would be used to indicate errors.

### Copyright

Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).
36 changes: 36 additions & 0 deletions http-gateways/PATH_GATEWAY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ where client prefers to perform all validation locally.
- [`Accept` (request header)](#accept-request-header)
- [`Range` (request header)](#range-request-header)
- [`Service-Worker` (request header)](#service-worker-request-header)
- [`X-Error-Behavior` (request header)](#x-error-behavior-request-header)
- [Request Query Parameters](#request-query-parameters)
- [`filename` (request query parameter)](#filename-request-query-parameter)
- [`download` (request query parameter)](#download-request-query-parameter)
Expand Down Expand Up @@ -74,6 +75,8 @@ where client prefers to perform all validation locally.
- [`X-Ipfs-Roots` (response header)](#x-ipfs-roots-response-header)
- [`X-Content-Type-Options` (response header)](#x-content-type-options-response-header)
- [`X-Trace-Id` (response header)](#x-trace-id-response-header)
- [`X-On-Error` (response header)](#x-on-error-response-header)
- [`X-Stream-Error` (response header)](#x-stream-error-response-header)
- [Response Payload](#response-payload)
- [Appendix: notes for implementers](#appendix-notes-for-implementers)
- [Content resolution](#content-resolution)
Expand All @@ -83,6 +86,7 @@ where client prefers to perform all validation locally.
- [Best practices for HTTP caching](#best-practices-for-http-caching)
- [Denylists](#denylists)
- [Generated HTML with directory index](#generated-html-with-directory-index)
- [Streaming errors](#streaming-errors)

# HTTP API

Expand Down Expand Up @@ -217,6 +221,17 @@ Gateway should refuse attempts to register a service worker for entire
Requests to these paths with `Service-Worker: script` MUST be denied by
returning HTTP 400 Bad Request error.

### `X-Error-Behavior` (request header)

Clients can request a different behavior when errors occur during an HTTP
stream. The possible values, and behaviors are defined as follows:

- `reset` (default): resets the HTTP stream.
- `trailer`: a trailer header [`X-Stream-Error`](#x-stream-error-response-header) will
be sent with the error message.

See the [streaming errors](#streaming-errors) section for more information.

## Request Query Parameters

All query parameters are optional.
Expand Down Expand Up @@ -582,6 +597,17 @@ unique identifier to help in debugging errors and performance issues.

A good practice is to always return it with HTTP error [status codes](#response-status-codes) >=`400`.

### `X-On-Error` (response header)

Returned on streaming requests to indicate the server behavior in case an error happens.
The header can have the value `reset`, or `trailer`. See the [streaming errors](#streaming-errors)
section for more information.

### `X-Stream-Error` (response header)

Returned if the client set [`X-Error-Behavior`](#x-error-behavior-request-header) to `trailer`. This trailing header will
contain the error message that occurred while streaming a file.

## Response Payload

Data sent with HTTP response depends on the type of requested IPFS resource:
Expand Down Expand Up @@ -705,3 +731,13 @@ The usual optimizations involve:
limiting the cost of a single page load.
- The downside of this approach is that it will always be slower than
skipping child block resolution.

## Streaming errors

To avoid users receiving an incomplete, yet valid, files, the gateway MUST
close the HTTP stream if an error occurs while streaming a file to the client.
This can be done via the following mechanisms:

- Sending a `FIN` (close) frame for HTTP/1.1
- Sending a `RST_STREAM` (reset stream) frame for HTTP/2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Sending a `RST_STREAM` (reset stream) frame for HTTP/2
- Sending a `RST_STREAM` (reset stream) frame for HTTP/2
- Sending a `CANCEL_PUSH` frame for HTTP/3, see RFC9114 section A.2.5.3.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't read the entire spec, but according to BCP 56, HTTP APIs shouldn't go into this level of detail. HTTP is designed in a way that allows building applications without referring to the specific HTTP version.

Copy link
Member

Choose a reason for hiding this comment

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

@Jorropo Again, haven't read the entire spec, but I are we actually doing server push (which is the only context in which CANCEL_PUSH would be valid)? That feature of HTTP is going to be removed from major browser implementations very soon.

Copy link
Contributor

@Jorropo Jorropo Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't actually red the RFC, I know RST_STREAM on HTTP/2 is what we want, I just CTRL + F RST_STREAM in the HTTP/3 RFC and found this text:

RST_STREAM (0x03): RST_STREAM frames do not exist in HTTP/3, since
QUIC provides stream lifecycle management. The same code point is
used for the CANCEL_PUSH frame (Section 7.2.3).

I guess it should be whatever QUIC's frame is to close a stream unexpectedly.

HTTP is designed in a way that allows building applications without referring to the specific HTTP version.

Streaming errors is not a thing HTTP supports, you can setup trailler headers but they aren't supported by browsers.
So we are stealing the rug from under HTTP and use the underlying protocol to generate an unexpected error (which is actually supported and will return errors in all client implementations I've tested), that why we need to use TCP FIN.

I don't know if we should add this to the gateway spec, because most HTTP server implementations give you as much control as the go std does.
How will you implement that on reverse proxies ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@marten-seemann as @Jorropo mentioned, the big issue here is that HTTP doesn't have any error handling for streaming. One of my motivations behind this IPIP regards the new TAR format for the gateway (ipfs/kubo#9029).

It is possible that the TAR creation fails while streaming the file to the client due to many reasons. However, if you just stop streaming the TAR, you still get a valid TAR file, but it is incomplete. There's no feedback. Printing a trailer header is useless since browsers are not able to parse them. The only way we found so far to tell the user that something is wrong was by force-closing the HTTP stream.

I also have mixed feelings about having this on the spec since it is so specific. In addition, as @Jorropo mentioned it may be worth it having an opt-out of the behaviour through some header.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hacdias

How will you implement that on reverse proxies ?

I think this is an important question.
I would like to see something like: X-On-Error: reset header sent by the server to indicate this will be done, then a reverse proxy could just not send thoses headers, and clients would be able to tell it is not gonna do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Jorropo please see the updates I've made to the IPIP.

- Sending a `CANCEL_PUSH` frame for HTTP/3, see RFC9114 section A.2.5.3.