[core] CheckValidSockets no longer remove sockets from group, this was causing inconsistency. #1687
Conversation
This comment has been minimized.
This comment has been minimized.
maxsharabayko
added
[core]
| d->sndstate = SRT_GST_BROKEN; | ||
| d->rcvstate = SRT_GST_BROKEN; |
There was a problem hiding this comment.
Are we safe to set this to broken? This happens under these locks:
m_pGlobal->m_GlobControlLockm_GroupLock
There was a problem hiding this comment.
Correct. The first one protects the sockets against being updated against the group (that is, removed from the group container in the meantime), the second one protects the m_Group container (members container). Note also that this function is being called from inside the sending function only, so it happens in the same thread as the transmission functions. A simultaneous call to a receiving function, let's say, isn't possible because it also requires lock on m_Group.
srtcore/group.cpp
Outdated
| CUDT* pu = 0; | ||
| if (d->ps) | ||
| pu = &d->ps->core(); |
There was a problem hiding this comment.
Might be better.
| CUDT* pu = 0; | |
| if (d->ps) | |
| pu = &d->ps->core(); | |
| const CUDT* pu = (d->ps) | |
| ? &d->ps->core() | |
| : NULL; |
There was a problem hiding this comment.
I believe you meant CUDT* const pu = (d->ps) ...
There was a problem hiding this comment.
Well, both. pu is not modified here, so const CUDT* const pu =....
There was a problem hiding this comment.
Right. In practice actually, this intermediate variable is only for having a shortcut - the condition would be more elaborate without it.
srtcore/group.cpp
Outdated
| CUDT* pu = 0; | ||
| if (d->ps) | ||
| pu = &d->ps->core(); |
There was a problem hiding this comment.
const pu... like above
The problem: CheckValidSockets called CUDTGroup::remove (locked version) for a socket that it couldn't find in the global container. This, however, might cause that a socket already moved to ClosedSockets will be left with a dangling iterator to the group container entry.