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: Delete allocation on TCP broken pipe #336

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

rg0now
Copy link
Contributor

@rg0now rg0now commented Jun 16, 2023

It seems many TURN/TCP clients out there implement a rather brute-force way to end TURN/TCP allocations: they spontaneously close the TCP connection underlying the allocation rather than properly closing it down by refreshing with zero lifetime. The server must delete the TURN allocation when it sees the client TCP connection has gone, otherwise the allocation will linger on and next time the client try to allocate from the same IP:port pair they receive a relay already allocated for 5-TUPLE error. Our users are actively being affected by this.

This PR fixes this issue and adds a test. The easiest way to understand the issue is through the test:

t.Run("Delete allocation on spontaneous TCP close", func(t *testing.T) {
    ...
    // Create TCP server
    server, err := NewServer(ServerConfig{ ... })

    // Create TCP client
    conn, err := net.Dial("tcp", "127.0.0.1:3478")
    client, err := NewClient(&ClientConfig{
        STUNServerAddr: serverAdddr,
        Conn:           NewSTUNConn(conn),
        ...
    })

    client.Listen()
    relayConn, err := client.Allocate()

    // if we close the relayConn, everything is fine
    relayConn.Close()

    // but if we close the underlying conn like this, the allocation lingers and the subsequent
    // tests fail
    conn.Close()
    ...
})

The issue is fixed by explicitly deleting the allocation in the Server.readListener when the readLoop returns.

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1f1673d) 67.33% compared to head (64c5353) 67.31%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   67.33%   67.31%   -0.03%     
==========================================
  Files          42       42              
  Lines        2841     2848       +7     
==========================================
+ Hits         1913     1917       +4     
- Misses        766      769       +3     
  Partials      162      162              
Flag Coverage Δ
go 67.31% <100.00%> (-0.03%) ⬇️
wasm 48.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rg0now
Copy link
Contributor Author

rg0now commented Jun 16, 2023

On a related note: I plan to submit a PR to remove FiveTuple.Protocol all together. Currently it is fixed at UDP irrespectively of the type of the listener, which is clearly wrong. Alas, there is no easy way to fix it: the server simply has no way to know the protocol it runs on top of. We could hack something together to fix the Protocol to UDP when we're running on top of a PacketConn and TCP otherwise, but this would also be wrong: one can easily wrap a WebSocket in a PacketConn and then we're back to square one.

Wdyt?

@rg0now rg0now force-pushed the fix-spontaneous-tcp-close branch from 428b9a7 to c3da752 Compare June 17, 2023 17:45
server.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
Explicitly delete allocation when a TURN/TCP client ends a TURN
allocation by spontaneously closing the underlying TCP
connection instead of properly closing it down by
refreshing with zero lifetime.
@rg0now rg0now merged commit 6a55914 into pion:master Nov 17, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants