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

Jump to bottom

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 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion IPIP/0000-gateway-error-handling.md
View file
Open in desktop
Expand Up @@ -41,7 +41,7 @@ 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 connection, leading to a network
Therefore, the server can force-close the HTTP stream, leading to a network
error. This tells the user that an error happened.

### User benefit
Expand Down
8 changes: 6 additions & 2 deletions http-gateways/PATH_GATEWAY.md
View file
Open in desktop
Expand Up @@ -701,5 +701,9 @@ The usual optimizations involve:

## Streaming errors

Gateways MUST force-close HTTP connections if they detect an error while
streaming a file to avoid that users receive incomplete, yet valid, files.
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 `RST` (reset) frame for HTTP/1.1
hacdias marked this conversation as resolved.
Show resolved Hide resolved
- 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

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.