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

Add fin and reset_stream fields #396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Feb 19, 2024

Fixes #375.

Per the discussion, it can be useful to know when an application triggers a
stream close, or when it receives one, without having to do offset/length maths.

I omitted stop_sending in this commit because it seems a little bit of a stretch,
mainly because it's wire image has no offset or final size. Also, STOP_SENDING
relates to asking the peer to do something, which further complicates the
(potential) use of offset and length fields.

An alternative would be to keep fin here but define new events for reset and
stop. Those could both contain from, to and error fields but only the reset
would have offset and length.

Fixes #375.

Per the discussion, it can be useful to know when an application triggers a
stream close, or when it receives one, without having to do offset/length maths.

I omitted stop_sending in this commit because it seems a little bit of a stretch,
mainly because it's wire image has no offset or final size. Also, STOP_SENDING
relates to asking the peer to do something, which further complicates the
(potential) use of offset and length fields.

An alternative would be to keep `fin` here but define new events for reset and
stop. Those could both contain from, to and error fields but only the reset
would have offset and length.
@rmarx
Copy link
Contributor

rmarx commented Feb 19, 2024

Discussed during editorial meeting:
Keep fin here, move reset_stream and stop_sending to separate event(s) (possibly having 2 because reset_stream is so different from stop_sending).

@marten-seemann it would help to have a quick mockup of what reset_stream_at might look like (not because we want to add it to main qlog yet, but if the current design would be compatible with that, it would be nice :)

@rmarx
Copy link
Contributor

rmarx commented Feb 28, 2024

Actually discussed further during meeting, opinions changed :)

I'm now in favor of something quite general, say:

? additional_info: [+ text]

; possible values for QUIC/H3 currently are "fin_set", "stream_reset", "stream_reset_at", "stop_sending"

With the idea that other events can be used to determine what exactly happened (e.g., look at the actual STREAM_RESET frame) and keep this event what it was originally intended for: showing when which type of data (and now info) is communicated between the layers (as this doesn't always coincide with packet sent/received)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve quic:stream_data_moved
2 participants