feat(s2n-quic-core): add MtuConfigError with full diagnostic context - #2946
Open
mrwatts88 wants to merge 3 commits into
Open
feat(s2n-quic-core): add MtuConfigError with full diagnostic context#2946mrwatts88 wants to merge 3 commits into
mrwatts88 wants to merge 3 commits into
Conversation
mrwatts88
marked this pull request as draft
January 24, 2026 19:20
mrwatts88
marked this pull request as ready for review
January 24, 2026 19:30
maddeleine
self-requested a review
January 26, 2026 19:05
Contributor
|
Hi, thanks for the contribution. Can you tell us more about your MTU usecase that drove the need for this PR? |
Author
I didn't have a specific use case, I just found an open issue. |
boquan-fang
reviewed
Feb 25, 2026
boquan-fang
left a comment
Contributor
There was a problem hiding this comment.
Hi @mrwatts88, thanks for contributing!
Do you mind adding a test or show us how would the MtuConfigError be displayed? Thanks!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Release Summary:
The
mtu::Manager::config()method now returnsResult<Config, MtuConfigError>instead ofResult<Config, MtuError>. This is a breaking change for code that handles errors from this method, as error handling code will need to be updated to handle the newMtuConfigErrortype. The new error type provides better diagnostic context including the remote address, connection-specific MTU config, and endpoint MTU config when validation fails.Resolved issues:
resolves #2254
Description of changes:
Previously, when MTU configuration validation failed in
mtu::Manager::config(), it returned a genericMtuErrorthat only provided a static message about the valid MTU range constraints. This made it difficult to diagnose why a specific configuration failed, especially in production environments where you need to know which remote address triggered the error and what the actual configuration values were.This PR introduces a new
MtuConfigErrortype that captures full diagnostic context:remote_addr: The remote socket address that caused the validation failure (ownedinet::SocketAddressto avoid lifetime complications withSocketAddress<'a>)conn_config: The connection-specific MTU configuration that was invalidendpoint_config: The endpoint's MTU configuration for comparisonImplementation strategy:
MtuConfigErrortype rather than extendingMtuErrorbecause these are fundamentally different error scenarios:MtuErroris for build-time validation failures when constructing MTU config, whileMtuConfigErroris for runtime validation failures when applying config to a specific pathMtuErrorinet::SocketAddressinstead of the borrowedSocketAddress<'a>to avoid lifetime management complexity in error propagation. Since this is an error path (validation failure), the performance impact of cloning the socket address should be small.The
mtu::Manager::config()method signature changes fromResult<Config, MtuError>toResult<Config, MtuConfigError>, which is a breaking change requiring callers to update their error handling.Additional changes:
PartialEq,Eq,PartialOrd, andOrdderives to MTU types (MaxMtu,InitialMtu,BaseMtu) to enable comparison in error contextPartialEqandEqderives tomtu::Configto support equality checks in diagnosticsDatagramDropReason::InvalidMtuConfigurationevent variant to includeconn_mtu_configandremote_addrfieldsThis change improves debuggability by providing actionable context when MTU configuration errors occur.
Call-outs:
Testing:
quic/s2n-quic-core/src/path/mtu/tests.rsto verify the newMtuConfigErrorfields contain the correct diagnostic informationremote_addr,conn_config, andendpoint_configare populated correctly when validation failsmtu::Manager::config()is caught at compile time - the single call site inquic/s2n-quic-transport/src/path/manager.rswas updated to use the new error typeDatagramDropReason::InvalidMtuConfigurationevent now includes the additional diagnostic fields (conn_mtu_configandremote_addr)All existing tests pass with the new error type, confirming the behavior is unchanged except for the improved diagnostic context.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.