diff --git a/agent_test.go b/agent_test.go index ad3dc1ed..a5ed200e 100644 --- a/agent_test.go +++ b/agent_test.go @@ -376,6 +376,134 @@ func TestAgentCloseAbortsBlockedUDPMuxWrite(t *testing.T) { require.NoError(t, <-runErr) } +func TestAgentCloseAbortsBlockedUDPMuxSrflxGatherWrite(t *testing.T) { + defer test.CheckRoutines(t)() + defer test.TimeOut(2 * time.Second).Stop() + + udpConn := newDeadlineBlockingPacketConn() + udpMux := NewUniversalUDPMuxDefault(UniversalUDPMuxParams{UDPConn: udpConn}) + defer func() { + _ = udpMux.Close() + }() + + agent, err := NewAgent(&AgentConfig{ + NetworkTypes: []NetworkType{NetworkTypeUDP4}, + CandidateTypes: []CandidateType{CandidateTypeServerReflexive}, + Urls: []*stun.URI{{ + Scheme: stun.SchemeTypeSTUN, + Host: "192.0.2.2", + Port: 3478, + }}, + UDPMuxSrflx: udpMux, + }) + require.NoError(t, err) + + require.NoError(t, agent.OnCandidate(func(Candidate) {})) + require.NoError(t, agent.GatherCandidates()) + + select { + case <-udpConn.writeStarted: + case <-time.After(time.Second): + require.FailNow(t, "timed out waiting for UDP mux srflx gather write to block") + } + + closeErr := make(chan error, 1) + go func() { + closeErr <- agent.Close() + }() + + select { + case err := <-closeErr: + require.NoError(t, err) + case <-time.After(time.Second): + require.FailNow(t, "agent close did not abort blocked UDP mux srflx gather write") + } +} + +func TestAgentCloseDoesNotAbortOtherAgentUDPMuxSrflxGatherWrite(t *testing.T) { //nolint:cyclop + defer test.CheckRoutines(t)() + defer test.TimeOut(2 * time.Second).Stop() + + udpConn := newDeadlineBlockingPacketConn() + udpMux := NewUniversalUDPMuxDefault(UniversalUDPMuxParams{UDPConn: udpConn}) + defer func() { + _ = udpMux.Close() + }() + + newSrflxAgent := func(t *testing.T) *Agent { + t.Helper() + + agent, err := NewAgent(&AgentConfig{ + NetworkTypes: []NetworkType{NetworkTypeUDP4}, + CandidateTypes: []CandidateType{CandidateTypeServerReflexive}, + Urls: []*stun.URI{{ + Scheme: stun.SchemeTypeSTUN, + Host: "192.0.2.2", + Port: 3478, + }}, + UDPMuxSrflx: udpMux, + }) + require.NoError(t, err) + + return agent + } + + agent1 := newSrflxAgent(t) + defer func() { + _ = agent1.Close() + }() + + agent2 := newSrflxAgent(t) + defer func() { + _ = agent2.Close() + }() + + require.NoError(t, agent2.OnCandidate(func(Candidate) {})) + require.NoError(t, agent2.GatherCandidates()) + + select { + case <-udpConn.writeStarted: + case <-time.After(time.Second): + require.FailNow(t, "timed out waiting for second agent UDP mux srflx gather write to block") + } + + closeErr := make(chan error, 1) + go func() { + closeErr <- agent1.Close() + }() + + select { + case err := <-closeErr: + require.NoError(t, err) + case <-time.After(time.Second): + require.FailNow(t, "first agent close blocked") + } + + select { + case <-udpConn.writeDeadlineSet: + require.FailNow(t, "first agent close aborted another agent's UDP mux srflx gather write") + case <-time.After(200 * time.Millisecond): + } + + secondCloseErr := make(chan error, 1) + go func() { + secondCloseErr <- agent2.Close() + }() + + select { + case <-udpConn.writeDeadlineSet: + case <-time.After(time.Second): + require.FailNow(t, "second agent close did not abort its own UDP mux srflx gather write") + } + + select { + case err := <-secondCloseErr: + require.NoError(t, err) + case <-time.After(time.Second): + require.FailNow(t, "second agent close blocked") + } +} + func TestAgentCloseClearsSharedUDPMuxAbortDeadlineForOtherAgent(t *testing.T) { //nolint:cyclop defer test.CheckRoutines(t)() defer test.TimeOut(2 * time.Second).Stop() diff --git a/gather.go b/gather.go index 6b9573b5..59f76fc2 100644 --- a/gather.go +++ b/gather.go @@ -760,7 +760,7 @@ func (a *Agent) gatherCandidatesSrflxUDPMux(ctx context.Context, urls []*stun.UR return } - xorAddr, err := a.udpMuxSrflx.GetXORMappedAddr(serverAddr, a.stunGatherTimeout) + xorAddr, err := getXORMappedAddr(ctx, a.udpMuxSrflx, serverAddr, a.stunGatherTimeout) if err != nil { a.log.Warnf("Failed get server reflexive address %s %s: %v", network, url, err) @@ -809,6 +809,27 @@ func (a *Agent) gatherCandidatesSrflxUDPMux(ctx context.Context, urls []*stun.UR } } +type contextXORMappedAddrGetter interface { + GetXORMappedAddrContext(context.Context, net.Addr, time.Duration) (*stun.XORMappedAddress, error) +} + +func getXORMappedAddr( + ctx context.Context, + mux UniversalUDPMux, + serverAddr net.Addr, + deadline time.Duration, +) (*stun.XORMappedAddress, error) { + if muxWithContext, ok := mux.(contextXORMappedAddrGetter); ok { + return muxWithContext.GetXORMappedAddrContext(ctx, serverAddr, deadline) + } + + if err := ctx.Err(); err != nil { + return nil, err + } + + return mux.GetXORMappedAddr(serverAddr, deadline) +} + //nolint:cyclop,gocognit func (a *Agent) gatherCandidatesSrflx(ctx context.Context, urls []*stun.URI, networkTypes []NetworkType) { var wg sync.WaitGroup diff --git a/gather_test.go b/gather_test.go index fad9b5b2..fd437253 100644 --- a/gather_test.go +++ b/gather_test.go @@ -1522,7 +1522,7 @@ func TestGatherCandidatesRelayRespectsInterfaceFilter(t *testing.T) { }, }), WithInterfaceFilter(func(iface string) bool { - return iface == "eth0" + return iface == "eth0" //nolint:goconst }), WithIncludeLoopback(), ) diff --git a/udp_mux.go b/udp_mux.go index 6b49e551..dd0f76db 100644 --- a/udp_mux.go +++ b/udp_mux.go @@ -4,6 +4,7 @@ package ice import ( + "context" "errors" "io" "net" @@ -50,7 +51,11 @@ type UDPMuxDefault struct { // whether the UDP connection listens on an unspecified address isUnspecified bool - // writeState packs write in-flight count. + // writeState coordinates context cancellation for WriteTo calls. + // Low bits count writes currently inside UDPConn.WriteTo. blocked means an + // abort arming the shared write deadline, so new writes wait. + // deadline means SetWriteDeadline(time.Now()) succeeded and the last + // in-flight writer must clear it before new writes can enter. writeState atomic.Uint64 } @@ -269,13 +274,53 @@ func (m *UDPMuxDefault) Close() error { } func (m *UDPMuxDefault) writeTo(buf []byte, rAddr net.Addr) (n int, err error) { - m.startWrite() + return m.writeToContext(context.Background(), buf, rAddr) +} + +func (m *UDPMuxDefault) writeToContext(ctx context.Context, buf []byte, rAddr net.Addr) (n int, err error) { + if err = m.startWriteContext(ctx); err != nil { + return 0, err + } defer func() { err = m.finishWrite(err) }() - return m.params.UDPConn.WriteTo(buf, rAddr) + if err = ctx.Err(); err != nil { + return 0, err + } + + if done := ctx.Done(); done != nil { + // net.PacketConn writes cannot be canceled directly. If ctx is + // canceled while WriteTo is blocked, abortWrite interrupts it by + // temporarily setting the shared socket write deadline to now. + stopAbort := make(chan struct{}) + var stopped atomic.Bool + defer func() { + stopped.Store(true) + close(stopAbort) + }() + go func() { + select { + case <-done: + if !stopped.Load() { + if abortErr := m.abortWrite(); abortErr != nil { + m.params.Logger.Warnf("Failed to abort UDP write: %v", abortErr) + } + } + case <-stopAbort: + } + }() + } + + n, err = m.params.UDPConn.WriteTo(buf, rAddr) + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return n, ctxErr + } + } + + return n, err } func (m *UDPMuxDefault) abortWrite() error { @@ -289,6 +334,8 @@ func (m *UDPMuxDefault) abortWrite() error { continue } + // The deadline applies to the shared UDPConn, so blocked stays set + // until the final in-flight writer clears the deadline in finishWrite. if err := m.params.UDPConn.SetWriteDeadline(time.Now()); err != nil { m.clearWriteAbortState() @@ -301,8 +348,12 @@ func (m *UDPMuxDefault) abortWrite() error { } } -func (m *UDPMuxDefault) startWrite() { +func (m *UDPMuxDefault) startWriteContext(ctx context.Context) error { for { + if err := ctx.Err(); err != nil { + return err + } + state := m.writeState.Load() if state&udpMuxWriteBlockedBit != 0 { runtime.Gosched() @@ -311,7 +362,7 @@ func (m *UDPMuxDefault) startWrite() { } if m.writeState.CompareAndSwap(state, state+1) { - return + return nil } } } @@ -357,6 +408,8 @@ func (m *UDPMuxDefault) clearWriteDeadlineAfterAbort(writeErr error) error { return writeErr } if state&udpMuxWriteDeadlineBit == 0 { + // The last writer can race with abortWrite after blocked is set but + // before SetWriteDeadline returns. runtime.Gosched() continue diff --git a/udp_mux_universal.go b/udp_mux_universal.go index 54d0aaad..7a00a80f 100644 --- a/udp_mux_universal.go +++ b/udp_mux_universal.go @@ -4,6 +4,7 @@ package ice import ( + "context" "fmt" "net" "time" @@ -177,55 +178,83 @@ func (m *UniversalUDPMuxDefault) GetXORMappedAddr( serverAddr net.Addr, deadline time.Duration, ) (*stun.XORMappedAddress, error) { - m.mu.Lock() - mappedAddr, ok := m.xorMappedMap[serverAddr.String()] - // If we already have a mapping for this STUN server (address already received) - // and if it is not too old we return it without making a new request to STUN server - if ok { - if mappedAddr.expired() { - mappedAddr.closeWaiters() - delete(m.xorMappedMap, serverAddr.String()) - ok = false - } else if mappedAddr.pending() { - ok = false - } - } - m.mu.Unlock() - if ok { - return mappedAddr.addr, nil + return m.GetXORMappedAddrContext(context.Background(), serverAddr, deadline) +} + +func (m *UniversalUDPMuxDefault) GetXORMappedAddrContext( + ctx context.Context, + serverAddr net.Addr, + deadline time.Duration, +) (*stun.XORMappedAddress, error) { + if mappedAddr, ok := m.cachedXORMappedAddr(serverAddr); ok { + return mappedAddr, nil } // Otherwise, make a STUN request to discover the address // or wait for already sent request to complete - waitAddrReceived, err := m.writeSTUN(serverAddr) + waitAddrReceived, err := m.writeSTUN(ctx, serverAddr) if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + return nil, fmt.Errorf("%w: %s", errWriteSTUNMessage, err) //nolint:errorlint } + timer := time.NewTimer(deadline) + defer timer.Stop() + // Block until response was handled by the connWorker routine and XORMappedAddress was updated select { case <-waitAddrReceived: // When channel closed, addr was obtained m.mu.Lock() - mappedAddr := *m.xorMappedMap[serverAddr.String()] - m.mu.Unlock() - if mappedAddr.addr == nil { + mappedAddr, ok := m.xorMappedMap[serverAddr.String()] + if !ok || mappedAddr.addr == nil { + m.mu.Unlock() + return nil, errNoXorAddrMapping } + addr := mappedAddr.addr + m.mu.Unlock() - return mappedAddr.addr, nil - case <-time.After(deadline): + return addr, nil + case <-timer.C: return nil, errXORMappedAddrTimeout + case <-ctx.Done(): + return nil, ctx.Err() + } +} + +func (m *UniversalUDPMuxDefault) cachedXORMappedAddr(serverAddr net.Addr) (*stun.XORMappedAddress, bool) { + m.mu.Lock() + defer m.mu.Unlock() + + mappedAddr, ok := m.xorMappedMap[serverAddr.String()] + // If we already have a mapping for this STUN server (address already received) + // and if it is not too old we return it without making a new request to STUN server + if !ok { + return nil, false } + if mappedAddr.expired() { + mappedAddr.closeWaiters() + delete(m.xorMappedMap, serverAddr.String()) + + return nil, false + } + if mappedAddr.pending() { + return nil, false + } + + return mappedAddr.addr, true } // writeSTUN sends a STUN request via UDP conn. // // The returned channel is closed when the STUN response has been received. // Method is safe for concurrent use. -func (m *UniversalUDPMuxDefault) writeSTUN(serverAddr net.Addr) (chan struct{}, error) { +func (m *UniversalUDPMuxDefault) writeSTUN(ctx context.Context, serverAddr net.Addr) (chan struct{}, error) { m.mu.Lock() - defer m.mu.Unlock() // If record present in the map, we already sent a STUN request, // just wait when waitAddrReceived will be closed @@ -237,17 +266,27 @@ func (m *UniversalUDPMuxDefault) writeSTUN(serverAddr net.Addr) (chan struct{}, } m.xorMappedMap[serverAddr.String()] = addrMap } + waitAddrReceived := addrMap.waitAddrReceived + m.mu.Unlock() req, err := stun.Build(stun.BindingRequest, stun.TransactionID) if err != nil { return nil, err } - if _, err = m.params.UDPConn.WriteTo(req.Raw, serverAddr); err != nil { + if _, err = m.writePacket(ctx, req.Raw, serverAddr); err != nil { return nil, err } - return addrMap.waitAddrReceived, nil + return waitAddrReceived, nil +} + +func (m *UniversalUDPMuxDefault) writePacket(ctx context.Context, packet []byte, addr net.Addr) (int, error) { + if m.UDPMuxDefault != nil && m.UDPMuxDefault.params.UDPConn != nil { + return m.UDPMuxDefault.writeToContext(ctx, packet, addr) + } + + return m.params.UDPConn.WriteTo(packet, addr) } type xorMapped struct { diff --git a/udp_mux_universal_test.go b/udp_mux_universal_test.go index 8de077e1..96de2235 100644 --- a/udp_mux_universal_test.go +++ b/udp_mux_universal_test.go @@ -403,3 +403,73 @@ func TestUniversalUDPMux_GetXORMappedAddr_WaitClosed_NoAddr(t *testing.T) { require.Nil(t, addr) require.ErrorIs(t, err, errNoXorAddrMapping) } + +func TestUniversalUDPMux_GetXORMappedAddr_WaitClosed_DeletedMapping(t *testing.T) { + serverAddr := &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 3478} + waitCh := make(chan struct{}) + + var mux *UniversalUDPMuxDefault + pc := &writeHookPacketConn{ + local: &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 1}, + onWrite: func() { + mux.mu.Lock() + mappedAddr := mux.xorMappedMap[serverAddr.String()] + mappedAddr.closeWaiters() + delete(mux.xorMappedMap, serverAddr.String()) + mux.mu.Unlock() + }, + } + mux = &UniversalUDPMuxDefault{ + UDPMuxDefault: &UDPMuxDefault{}, + params: UniversalUDPMuxParams{ + UDPConn: pc, + }, + xorMappedMap: map[string]*xorMapped{ + serverAddr.String(): { + waitAddrReceived: waitCh, + expiresAt: time.Now().Add(time.Minute), + }, + }, + } + + addr, err := mux.GetXORMappedAddr(serverAddr, time.Second) + require.Nil(t, addr) + require.ErrorIs(t, err, errNoXorAddrMapping) +} + +type writeHookPacketConn struct { + local net.Addr + onWrite func() +} + +func (w *writeHookPacketConn) ReadFrom([]byte) (int, net.Addr, error) { + return 0, w.local, io.EOF +} + +func (w *writeHookPacketConn) WriteTo(p []byte, _ net.Addr) (int, error) { + if w.onWrite != nil { + w.onWrite() + } + + return len(p), nil +} + +func (w *writeHookPacketConn) Close() error { + return nil +} + +func (w *writeHookPacketConn) LocalAddr() net.Addr { + return w.local +} + +func (w *writeHookPacketConn) SetDeadline(time.Time) error { + return nil +} + +func (w *writeHookPacketConn) SetReadDeadline(time.Time) error { + return nil +} + +func (w *writeHookPacketConn) SetWriteDeadline(time.Time) error { + return nil +}