connmgr: fix transport association bug

[sukunrt]

The second listen for the same IP:Port combination doesn't get tagged with the association. So if you listen on webtransport after quic, the wt dials won't reuse the transport.

We should refactor this is bit more, but any change will probably change the refCountedTransport interface which is returned from one public method, so I'd like to do that in a separate PR, and use this for a patch release.

This probably impacts: #3165

[ethan-gallant]

@MarcoPolo I have confirmed this resolves the issue and enables the hole punch to succeed.

[ethan-gallant]

@sukunrt can we merge this one?

[MarcoPolo]

I’ll review this on Monday. Thanks for tracking this down @sukunrt

[MarcoPolo]

Do we need this method? It's doing some sanity checks, but ultimately it just
calls tr.associate

[MarcoPolo]

Suggested change

	} else if c.enableReuseport && association != nil {
	reuse, err := c.getReuse(netw)
	if err != nil {
	return nil, fmt.Errorf("reuse error: %w", err)
	}
	err = reuse.AssociateListenTransport(association, entry.ln.transport)
	if err != nil {
	return nil, fmt.Errorf("reuse associate failed: %w", err)
	}
	} else if c.enableReuseport {
	if tr, ok := entry.ln.transport.(*refcountedTransport); ok {
	tr.associate(association)
	}

[sukunrt]

What's wrong with sanity checks?

• The reuse type isn't exported, so this method is effectively private to this package.

I'd argue for one of the two:

1. We move all associate inside reuse which may still require a similar method.
   or
2. We add Associate on the refCountedTransport interface with a NOOP implementation in singleOwnerTransport to avoid the type assertion.

But for a patch release, I think this method with sanity checks is fine.

[MarcoPolo]

Sanity checks are fine in general. These seem to be in the wrong place and thus add noise to the change. You are checking the reuse struct's invariants. Is there a reason you don't trust these invariants? Would it be better to do these checks where we would break the invariants instead (here)?

If you do trust these invariants, then the sanity checks here are just noise. and the above suggestion is a clearer change.

[MarcoPolo]

I think something like this would also be okay:

Suggested change

	} else if c.enableReuseport && association != nil {
	reuse, err := c.getReuse(netw)
	if err != nil {
	return nil, fmt.Errorf("reuse error: %w", err)
	}
	err = reuse.AssociateListenTransport(association, entry.ln.transport)
	if err != nil {
	return nil, fmt.Errorf("reuse associate failed: %w", err)
	}
	} else if c.enableReuseport {
	// ... get reuse
	err := reuse.AssertTransportExists(tr) // This method _only_ checks invariants
	// ... handle err
	if tr, ok := entry.ln.transport.(*refcountedTransport); ok {
	tr.associate(association)
	}

This split seems clearer to me because you have your sanity check step, and then your normal association step.

Note that I believe I would have also made the same initial design, and am mostly able to make this point as a fresh reviewer of this change.

[sukunrt]

Is there a reason you don't trust these invariants?

The input transport is provided from outside reuse. That user might pass in anything.

On the optimal design though, I've changed my mind. I think we should put associate on refCountedTransport and have a NO-Op implementation in singleOwnerTransport. Then we move the associate bit out of transportForListen in to this function. So associate always happens on all code paths no matter the input.
I'm not sure how semver compliant that is so I'll do that in a separate PR.

[MarcoPolo]

The input transport is provided from outside reuse. That user might pass in anything.

I don't follow. The transport here is from the quicListeners map which is controlled by this quicreuse package.

On the optimal design though, I've changed my mind. I think we should put associate on refCountedTransport and have a NO-Op implementation in singleOwnerTransport.

No strong opinion here, but I think it's fine to keep this method only on refCountedTransport as it's only meaningful there.

[MarcoPolo]

I want to add an actual hole punching test using the simconn for this as well.