Adding OnOpenComplete #122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
==========================================
+ Coverage 65.97% 67.96% +1.98%
==========================================
Files 4 4
Lines 194 206 +12
==========================================
+ Hits 128 140 +12
Misses 41 41
Partials 25 25
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -44,6 +45,10 @@ type DataChannel struct { | |||
| bytesSent uint64 | |||
| bytesReceived uint64 | |||
|
|
|||
| mu sync.Mutex | |||
There was a problem hiding this comment.
Would be great if we can reduce count of used mutexes as this is often source of bugs. Not sure though if it is possible in this case.
There was a problem hiding this comment.
@daonb why do need mutexes? As I see both code paths are invoked by library client.
There was a problem hiding this comment.
We need a mutext to protect from callback changes just as the library is calling the callback. onOpenComplete is called when the channel ack is received from the remote peer.
This pattern is copied from webrtc's OnDataChannel and it makes sense here as we shouldn't make any assumption as to when the client changes the callback.
|
I agree with you. But why can't we require that library client (pion/webrtc
?) should use the library in synchronous way?
If a client uses async functions, the client shall handle it themselves.
benny daon ***@***.***> schrieb am Mi., 27. Okt. 2021, 17:35:
… ***@***.**** commented on this pull request.
------------------------------
In datachannel.go
<#122 (comment)>:
> @@ -44,6 +45,10 @@ type DataChannel struct {
bytesSent uint64
bytesReceived uint64
+ mu sync.Mutex
We need a mutext to protect from callback changes just as the library is
calling the callback. onOpenComplete is called when the channel ack is
received from the remote peer.
This pattern is copied from webrtc's OnDataChannel and it makes sense here
as we shouldn't make any assumption as to when the client changes the
callback.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#122 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPTSYKGI43DAQZCPZDQXF3UJAS4PANCNFSM5GU7PGIQ>
.
|
|
LGTM! @ashellunts Pion has always provided a thread safe API. I don't think we will hit a lot of lock contention on this mutex, so this should be pretty safe! @daonb would you mind just squashing down into one commit? Thank you so much for the fix and thank you for the good reviews @ashellunts :) |
Thank you for your feedback, Sean! |
|
Thanks guys for the quick review. Be happy to squash & merge only WASM is failing. Is it the PR's fault? or is there some deep magic that will let me merge? |
Sean-Der
force-pushed
the
handleack
branch
2 times, most recently
from
8ea0d98
to
629ac6a
Compare
|
@ashellunts yea! I am debugging the WASM issue |
Add a OnOpenComplete handler. This allows us to fire OnOpen messages correctly in pion/webrtc Fixes pion#81 Relates to pion/webrtc#1063
Description
Following #81 and the discussion there, I copied the code from #56 and simplified it - the change now adds a special event handler to be used by the channel opener.
With the change, and a small
pion/webrtcPR which will soon follow, the peer that opens a channel can useOnOpenCompleteto set a handler. Unlike theOnOpenhandler, this handler can send data over the channel.Reference issue
Fixes #81