Skip to content
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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Feb 28, 2024

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 to m_ClosedSockets and after this happens the socket is no longer dispatchable through CUDTUnited::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 to m_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 in CUDT::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.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 64.81481% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 64.67%. Comparing base (4f925fb) to head (212750d).
Report is 9 commits behind head on master.

❗ Current head 212750d differs from pull request most recent head f56dba5. Consider uploading reports for the commit f56dba5 to get more accurate results

Files Patch % Lines
srtcore/api.cpp 52.00% 12 Missing ⚠️
srtcore/queue.cpp 50.00% 6 Missing ⚠️
srtcore/core.cpp 88.88% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@ethouris ethouris marked this pull request as ready for review March 13, 2024 09:49
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code labels May 7, 2024
@maxsharabayko maxsharabayko added this to the v1.5.4 milestone May 7, 2024
srtcore/api.h Outdated Show resolved Hide resolved
srtcore/api.h Outdated Show resolved Hide resolved
const bool drift_updated =
#endif
m_pRcvBuffer->addRcvTsbPdDriftSample(ctrlpkt.getMsgTimeStamp(), tsArrival, rtt);
//leaveCS(m_RcvBufferLock);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
@@ -8723,11 +8723,14 @@
// srt_recvfile (which doesn't make any sense), you'll have a deadlock.
if (m_config.bDriftTracer)
{
//enterCS(m_RcvBufferLock);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants