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

Remove HTTP_PUSH_ALREADY_IN_CACHE error #2813

Merged
merged 3 commits into from Jun 27, 2019

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Jun 19, 2019

Fixes #2812

@ianswett ianswett added the -http label Jun 19, 2019
Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

Is there a reason to reserve the error codes we remove?

{{errors}}). This asks the server not to transfer additional data and indicates
that it will be discarded upon receipt.
CANCEL_PUSH frame, a QUIC STOP_SENDING frame with an error code of
HTTP_PUSH_REFUSED. This asks the server not to transfer additional data and
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is missing a verb.

@LPardue
Copy link
Member Author

LPardue commented Jun 19, 2019

My intention was to keep PRs somewhat atomic and fragment the error code space in each PR that separately removes or consolidates an error code.

Once complete, I would then defrag it on a PR that let's people weigh in on which error code should have which number.

(It also simplifies dependencies which having branches off branches)

Happy to do something different if you want.

@@ -785,10 +785,9 @@ amount of data a server may commit to the pushed stream.

If a promised server push is not needed by the client, the client SHOULD send a
CANCEL_PUSH frame. If the push stream is already open or opens after sending the
CANCEL_PUSH frame, a QUIC STOP_SENDING frame with an appropriate error code can
Copy link
Member

Choose a reason for hiding this comment

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

To me the subject and the verb being omitted in this sentence was a bit surprising, but that might be due to me not being a native speaker.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is an error, thanks for picking it up. I would convert this to

"a QUIC STOP_SENDING frame with an error code of HTTP_REQUEST_CANCELLED can be used."

Alternatively, elsewhere we talk about "triggering QUIC STOP_SENDING" which is under review as part of @MikeBishop #2805

@@ -1903,9 +1901,9 @@ The entries in the following table are registered by this document.
| ----------------------------------- | ---------- | ---------------------------------------- | ---------------------- |
| HTTP_NO_ERROR | 0x0000 | No error | {{http-error-codes}} |
| HTTP_WRONG_SETTING_DIRECTION | 0x0001 | Setting sent in wrong direction | {{http-error-codes}} |
| HTTP_PUSH_REFUSED | 0x0002 | Client refused pushed content | {{http-error-codes}} |
| Reserved | 0x0002 | N/A | N/A |
Copy link
Member

Choose a reason for hiding this comment

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

My sense is that we want to do a final cleanup for reserved error codes. We might reserve some for keeping the relationship with RFC 7540 clean, but I don't think that a renumbering is a bad idea after all this work on fixing errors completes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. Not many good reasons exist to reserve codes for the shipped RFC.

@MikeBishop MikeBishop merged commit 6678903 into quicwg:master Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rationalise Server-Push-specific rejection error codes
5 participants