-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
webrtc: wait for FIN_ACK before closing data channels #2615
Merged
Merged
Changes from 16 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
e83480a
webrtc: wait for fin_ack for closing datachannel
sukunrt cba7577
fix conn closing on peerID mismatch
sukunrt 846c9ab
webrtc: don't test for ManyStreams in transport integration test
sukunrt f5b5982
remove lock for channel push
sukunrt 4865037
don't track streams on connection
sukunrt c2a30c7
set receive MTU
sukunrt d0ad152
debug log
sukunrt d21921b
webrtc: update pion
sukunrt 046d450
better debug log
sukunrt 29ed540
don't rely on pion for channel numbering
sukunrt 340d322
remove dependency on personal sctp fork
sukunrt 1e39ac5
remove unimportant changes
sukunrt b8578f2
update pion
sukunrt acf1687
add tests for chunking
sukunrt cffc6a8
use default MTU
sukunrt f6b86de
improve test for swarm stream
sukunrt bed82fe
fix sync stuff from review comments
sukunrt bc7390f
remove the onBufferedAmount callback to clear all references to stream
sukunrt bb4aea6
rename udpmux.ReceiveMTU to udpmux.ReceiveBufSize
sukunrt 3bff7d7
Merge branch 'master' into sukun/webrtc-fin-ack
sukunrt fbcb8f9
reuse msg for writes
sukunrt 5f0faa3
remove AsyncClose
sukunrt 17c89bd
go mod tidy
sukunrt 37ff2f6
return connection timed out errors on connection close for timeout
sukunrt bd78a36
add rationale for maxTotalControlMessagesSize
sukunrt 8abd3fe
fix race in connection timeout
sukunrt 058d7e9
Merge branch 'master' into sukun/webrtc-fin-ack
sukunrt 033ec87
fix check for timeout error
sukunrt ec4a973
remove redundant hanging handlers test
sukunrt 7e005b5
Merge branch 'master' into sukun/webrtc-fin-ack
sukunrt 0094df6
cleanup write path channels
sukunrt 2924291
use crypto/rand
sukunrt 8992b8c
disallow SetReadDeadline after read half is closed
sukunrt 801a23a
Merge branch 'master' into sukun/webrtc-fin-ack
sukunrt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| package swarm | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/libp2p/go-libp2p/core/network" | ||
| "github.com/libp2p/go-libp2p/core/peerstore" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type asyncStreamWrapper struct { | ||
| network.MuxedStream | ||
| beforeClose func() | ||
| done chan bool | ||
| } | ||
|
|
||
| func (s *asyncStreamWrapper) AsyncClose(onDone func()) error { | ||
| s.beforeClose() | ||
| err := s.Close() | ||
| go func() { | ||
| <-s.done | ||
| onDone() | ||
| }() | ||
| return err | ||
| } | ||
|
|
||
| func TestStreamAsyncCloser(t *testing.T) { | ||
| s1 := makeSwarm(t) | ||
| s2 := makeSwarm(t) | ||
|
|
||
| s1.Peerstore().AddAddrs(s2.LocalPeer(), s2.ListenAddresses(), peerstore.TempAddrTTL) | ||
|
|
||
| var mx sync.Mutex | ||
| var wg1, wg2 sync.WaitGroup | ||
| var closed int | ||
| done := make(chan bool) | ||
| const N = 100 | ||
| wg1.Add(N) | ||
| // wg2 blocks all goroutines in the beforeClose method. This allows us to check GetStreams | ||
| // works concurrently with Close | ||
| wg2.Add(N) | ||
| for i := 0; i < N; i++ { | ||
| go func() { | ||
| s, err := s1.NewStream(context.Background(), s2.LocalPeer()) | ||
| require.NoError(t, err) | ||
| ss, ok := s.(*Stream) | ||
| require.True(t, ok) | ||
| as := &asyncStreamWrapper{ | ||
| MuxedStream: ss.stream, | ||
| beforeClose: func() { | ||
| wg2.Done() | ||
| mx.Lock() | ||
| defer mx.Unlock() | ||
| closed++ | ||
| wg1.Done() | ||
| }, | ||
| done: done, | ||
| } | ||
| ss.stream = as | ||
| ss.Close() | ||
| }() | ||
| } | ||
| wg2.Wait() | ||
| require.Eventually(t, func() bool { return s1.Connectedness(s2.LocalPeer()) == network.Connected }, | ||
| 5*time.Second, 100*time.Millisecond) | ||
| require.Equal(t, len(s1.ConnsToPeer(s2.LocalPeer())[0].GetStreams()), N) | ||
|
|
||
| wg1.Wait() | ||
| require.Equal(t, closed, N) | ||
| // Streams should only be removed from the connection after the onDone call back is executed | ||
| require.Equal(t, len(s1.ConnsToPeer(s2.LocalPeer())[0].GetStreams()), N) | ||
|
|
||
| for i := 0; i < N; i++ { | ||
| done <- true | ||
| } | ||
| require.Eventually(t, func() bool { | ||
| return len(s1.ConnsToPeer(s2.LocalPeer())[0].GetStreams()) == 0 | ||
| }, 5*time.Second, 100*time.Millisecond) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docs on the method please, I had to read the implementation to understand this actually also started the close process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment explaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for explaining why it's private.
Just so you know this doesn't actually prevent anyone from implementing it.
You can use a badge using an internal package if you want to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why it reads "discourage" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems totally unnecessary. Why not just call
closeAndRemoveStreamimmediately? From the perspective of libp2p, the stream is closed. If callingcloseAndRemoveStreamcauses issues, that's a separate bug that needs to be fixed.Really, all stream closure is async, this isn't unique to webrtc.