Fix sctp crash #333
Fix sctp crash #333
Conversation
This reverts commit 4e13ff8.
Codecov Report
@@ Coverage Diff @@
## master #333 +/- ##
==========================================
+ Coverage 86.95% 87.04% +0.08%
==========================================
Files 33 33
Lines 8028 8044 +16
==========================================
+ Hits 6981 7002 +21
+ Misses 1047 1042 -5
Continue to review full report at Codecov.
|
| @@ -1,6 +1,8 @@ | |||
| #define LOG_CLASS "SCTP" | |||
| #include "../Include_i.h" | |||
|
|
|||
| static volatile PSctpSessionControl pSctpSessionControl = NULL; | |||
There was a problem hiding this comment.
We shouldn't use statics here. This can be part of the object graph. Please change the deinitSctpSession to take this object of a parent object that it can use instead of the global
There was a problem hiding this comment.
This thing out lives peerConnection, so we dont have a right parent object for it.
There was a problem hiding this comment.
I understand now your point. Let's make this a high-level object that's lifespan will be tracked at the application level.
Have createSctpSession and freeSctpSession. The object created then will be passed to the perr connections?
| @@ -63,15 +65,33 @@ STATUS initSctpSession() | |||
| // Disable Explicit Congestion Notification | |||
| usrsctp_sysctl_set_sctp_ecn_enable(0); | |||
|
|
|||
| pSctpSessionControl = MEMALLOC(SIZEOF(SctpSessionControl)); | |||
There was a problem hiding this comment.
The init and deinit can take an object/return an object to avoid the global. This object then can be part of the overall WebRTC object graph
There was a problem hiding this comment.
It will still be a global though right? It is shared among all PeerConnections
There was a problem hiding this comment.
Having mutable globals is a very bad idea at a high-level code. I think we might need to have a high-level object that lives in parallel with peer connection (like the signaling client) that will be passed into peer connection and it's lifecycle managed by the app
| @@ -63,15 +65,33 @@ STATUS initSctpSession() | |||
| // Disable Explicit Congestion Notification | |||
| usrsctp_sysctl_set_sctp_ecn_enable(0); | |||
|
|
|||
| pSctpSessionControl = MEMALLOC(SIZEOF(SctpSessionControl)); | |||
| pSctpSessionControl->lock = MUTEX_CREATE(FALSE); | |||
| CHK_STATUS(hashTableCreate(&pSctpSessionControl->activeSctpSessions)); | |||
There was a problem hiding this comment.
Use the parameterized version of the hash table. The default will create 2 * 10K buckets
|
|
||
| /* If the key is not in activeSctpSessions, then the session must have been freed. Thus do not call callback. | ||
| * Fixes issue stated here: https://github.com/sctplab/usrsctp/issues/147. | ||
| * (return -1 otherwise usrsctp_finish() won't finish) */ |
| struct socket *socket; | ||
| SctpSessionCallbacks sctpSessionCallbacks; | ||
| UINT64 key; |
There was a problem hiding this comment.
So you use store the current time here and this acts as the key in the hash table. Perhaps add a comment to state what you use as a key. Also, we really need to create a hashset object in PIC
|
Sorry for the push back, but I think this will just end up accruing more technical debt :/
|
|
PR open in develop branch |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.