Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IPIP-332: Streaming Error Handling on Web Gateways #332
IPIP-332: Streaming Error Handling on Web Gateways #332
Changes from 5 commits
8a175f6
a84fb63
4e73544
8ccb87d
9480caa
a20a665
e7f6b54
3e3b0fe
0fcdd02
4dd1293
c0c6af7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
I guess it should be whatever QUIC's frame is to close a stream unexpectedly.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hacdias
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.There was a problem hiding this comment.
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.