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

fix(s2n-quic-core): add path handle maybe_update method #1713

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Apr 17, 2023

Resolved issues:

resolves #954

Description of changes:

As noted in the linked issue, we are currently not updating the client's local path handle for the duration of the connection. This is a bigger problem when we get to XDP since we also need to store the Ethernet address for the remote handle, in addition to updating the local IP.

I've added a maybe_update function on the path handle trait so the client can update it on receiving a packet from the server.

Call outs

While I was here, I moved the limits range test into the actual limits module. The s2n-quic/src/tests.rs is for integration-level testing.

Testing:

You can see the output of the events now showing the local address on the client: https://dnglbrstg7yg.cloudfront.net/f020c737fc9f45497f3620669ff52639c6248181/interop/logs/latest/aioquic_s2n-quic/ipv6/client/logs.txt

Before it wasn't: https://dnglbrstg7yg.cloudfront.net/95db1f7e16463c77880cb69fb9e64e43f692c1ba/interop/logs/latest/aioquic_s2n-quic/ipv6/client/logs.txt

I've also included an integration test to make sure the behavior is preserved.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft marked this pull request as ready for review April 18, 2023 03:27
mod tests {
use super::*;

// Local max data limits should be <= u32::MAX

Choose a reason for hiding this comment

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

Suggested change
// Local max data limits should be <= u32::MAX
// The default Local max data limits should be <= u32::MAX

When reading this code my first question was: why doesn't with_data_window have the type signature: with_data_window(_, u32) -> Result<_, _>?. Presumably its because we do want to be able set the limit higher that u32::MAX. I read this comment as a contradiction: "Local max data limit should always be <= u32::MAX".

If I read the comment correctly: then my question I ask why does with_data_window have the signature it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code comes from #1693 and I'm just moving it, since it doesn't really belong in this module.

To answer your question: we may increase the limit in the future. We also can't break the API so we can't change the size.

@camshaft camshaft merged commit 687da14 into main Apr 18, 2023
@camshaft camshaft deleted the camshaft/path-update branch April 18, 2023 15:29
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.

Research strategy to populate local_address when endpoint is a Client
2 participants