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 next Sequence Number to PATH_CIDS_BLOCKED #492

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

mirjak
Copy link
Collaborator

@mirjak mirjak commented Feb 19, 2025

No description provided.

@mirjak
Copy link
Collaborator Author

mirjak commented Feb 19, 2025

Note that this would be a wire-breaking change, therefore this PR also updates the transport parameter.

}
~~~
{: #fig-path-cid-blocked-frame-format title="PATH_CIDS_BLOCKED Frame Format"}

Path Identifier:
: Identifier of the path for which unused connection IDs are not available.

Maximum Sequence Number:
: The largest sequence number issued for a connection ID for this path by the peer.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the peer has not yet sent any CID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, we would make this the next expected CID sequence number. So effectively the largest+1 or 0 if no CID was issued yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. But then, what is supposed to happen if the peer receives a fanciful number?

Copy link
Contributor

Choose a reason for hiding this comment

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

Notice that having a sequence number does not by itself solve the repetition problem. Suppose the peer has not sent a CID yet. I observe that the packet has not been acked. Should I repeat the frame? What if this is a spurious repeat. Will I repeat the spurious repeat as long as the number has not increased?

In the end, I think I will have to keep the workaround that I had in the code -- a flag that says "there is an unacked CID blocked frame for this path", cleared when the frame is acked or when new CIDs arrive. But if I have that flag, the number of CID in the frame is just informational. The value is mostly to be nice with the receiver, as in "if the number expected is lower than what you have sent, don't bother." Kinda nice to have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly this change is to help the receive to figure what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think this is overall a good change, because it aligns the syntax of PATH_CIDS_BLOCKED with that of the very similar STREAM_DATA_BLOCKED. It helps processing at the receivers: they can ignore PATH_CIDS_BLOCKED if they have already sent more CIDs for this path than indicated by the sequence number.

The change does not completely solve the retransmission problem. It helps a bit -- no point repeating if the peer has already sent more CIDs. Endpoints still need to track acknowledgements if they want to avoid spurious retransmissions -- but then, that's true of every frame, so there may not be much need to say that again.

Copy link
Contributor

@huitema huitema left a comment

Choose a reason for hiding this comment

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

We have to resolve what MAX_PATH_ID_BLOCKED (path=1, sequence=0) actually means. Is it "the sender has received number 0 and expects number 1" or "the sender has not received number 0 and expects it"?

}
~~~
{: #fig-path-cid-blocked-frame-format title="PATH_CIDS_BLOCKED Frame Format"}

Path Identifier:
: Identifier of the path for which unused connection IDs are not available.

Maximum Sequence Number:
: The largest sequence number issued for a connection ID for this path by the peer.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think this is overall a good change, because it aligns the syntax of PATH_CIDS_BLOCKED with that of the very similar STREAM_DATA_BLOCKED. It helps processing at the receivers: they can ignore PATH_CIDS_BLOCKED if they have already sent more CIDs for this path than indicated by the sequence number.

The change does not completely solve the retransmission problem. It helps a bit -- no point repeating if the peer has already sent more CIDs. Endpoints still need to track acknowledgements if they want to avoid spurious retransmissions -- but then, that's true of every frame, so there may not be much need to say that again.

@mirjak
Copy link
Collaborator Author

mirjak commented Feb 24, 2025

@huitema I prosed changes to rename to "Next Sequence Number". Does that work?

Copy link
Contributor

@qdeconinck qdeconinck left a comment

Choose a reason for hiding this comment

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

If the semantic of PATHS_BLOCKED is "I cannot open a new Path ID because of the peer limit" and PATH_CIDS_BLOCKED is "I cannot use a (valid) path with Path ID because there is no usable CID", then I agree with the suggestion to advertise the expected next sequence number -- receiving Next Seq Num 0 would be a clear indication that the host is willing to open a new path with a valid Path ID.

Co-authored-by: Quentin De Coninck <[email protected]>
@mirjak mirjak changed the title Add Maximum Sequence Number to PATH_CIDS_BLOCKED Add next Sequence Number to PATH_CIDS_BLOCKED Feb 25, 2025
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.

4 participants