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 SendDatagram #4436

Closed
wants to merge 4 commits into from
Closed

Conversation

nekohasekai
Copy link
Contributor

SendDatagram returned wrong PeerMaxDatagramFrameSize value

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Code looks correct to me, but can we have a test?

@nekohasekai
Copy link
Contributor Author

nekohasekai commented Apr 15, 2024

I found that there is already a test:

Expect(conn.SendDatagram(b)).To(MatchError(&quic.DatagramTooLargeError{
PeerMaxDatagramFrameSize: int64(maxDatagramMessageSize),
}))
.

But MatchError always returns true because DatagramTooLargeError implements Is(error) incorrectly

func (matcher *MatchErrorMatcher) Match(actual any) (success bool, err error) {
                ...

		// first try the built-in errors.Is
		if errors.Is(actualErr, expected.(error)) {
			return true, nil
		}

quic-go/errors.go

Lines 70 to 73 in 7b9d21f

func (e *DatagramTooLargeError) Is(target error) bool {
_, ok := target.(*DatagramTooLargeError)
return ok
}

@nekohasekai nekohasekai force-pushed the fix-send-datagram branch 2 times, most recently from e84d1f5 to a3bb384 Compare April 15, 2024 16:37
@nekohasekai
Copy link
Contributor Author

It almost passed. There is a race, but it doesn't seem to be related to my changes.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I don’t understand your changes regarding the errors. I also don’t see any test that’s testing your change.

@nekohasekai
Copy link
Contributor Author

nekohasekai commented Apr 17, 2024

These lines should test the PeerMaxDatagramFrameSize returned by SendDatagram but actually always pass because error has a wrong implementation of Is(error). According to the implementation of errors.Is , Is(error) should check for equality, not the type (that's what errors.As implements).

Expect(conn.SendDatagram(b)).To(MatchError(&quic.DatagramTooLargeError{
PeerMaxDatagramFrameSize: int64(maxDatagramMessageSize),
}))

@marten-seemann
Copy link
Member

That might be a separate issue. Please don't change the error implementation in this PR.

@marten-seemann
Copy link
Member

@nekohasekai Are you still working on this PR? Would be nice to get the fix into the v0.43 release.

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.

None yet

2 participants