-
Notifications
You must be signed in to change notification settings - Fork 863
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
[core] Added busy counter for sockets and various fixes for data race problems #2893
[core] Added busy counter for sockets and various fixes for data race problems #2893
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2893 +/- ##
==========================================
- Coverage 64.88% 64.67% -0.21%
==========================================
Files 101 101
Lines 17634 17565 -69
==========================================
- Hits 11441 11360 -81
- Misses 6193 6205 +12 ☔ View full report in Codecov by Sentry. |
3d108fa
to
11dd050
Compare
…nto dev-add-socket-busy-counter
…nto dev-add-socket-busy-counter
Here is introduced the SocketKeeper class, similar to GroupKeeper, together with the busy counter. The goal of this solution is to keep the socket alive until the busy counter reaches 0. This allows various facilities that are still running after the socket is requested to be closed to finish their job without having the socket been physically deleted in the meantime (in the code there were many places with a high risk of this to happen).
The overall procedure is that the socket when closed is being moved from
m_Sockets
tom_ClosedSockets
and after this happens the socket is no longer dispatchable throughCUDTUnited::locateSocket
. After this happens, the cyclic call to GC may potentially delete the socket. But there can be still some activities running in other threads that might use this socket even when simultaneously moved tom_ClosedSockets
. This temporary container is intended to be used for that exactly purpose, to keep a socket that should no longer be visible until all threads finish using it. Various checks are being done inCUDT::checkBrokenSockets
to prevent deletion of sockets that are still in use by other threads, but new features in SRT have added extra situations when this may happen and it has been observed in the ThreadSanitizer that - although not fatal - an attempt to delete a socket that is simultaneously used to send a packet was detected.To prevent this, in various longer procedures that might be potentially caught in the middle of deletion of a socket there is applied a SocketKeeper. This does RAII operation on the m_iBusy counter so that it is set back to the previous value only when this procedure is finished. The loop over the
m_ClosedSockets
that selects the sockets to delete will check this and if this busy counter is not zero, deletion of a socket will miss this cycle (will have to wait for the next GC cycle to try again).A small controversy might exist about the free acquire/release functions and that's why the use of them should be exclusive to SocketKeeper so that the RAII mechanism can ensure that a temporary busy counter will always be withdrawn when no longer needed, and no such situation is permanent.