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] Redesigned and fixed locking errors around CSndUList #1814

Closed

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Feb 17, 2021

Changes:

  1. The m_WindowLock and m_WindowCond has been moved from CSndQueue to CSndUList. This is actually something that works for the sake of the CSndUList object.
  2. The m_WindowLock and m_ListLock have been unified.
  3. The LOCK-CV pair has been implemented as common m_ListEv object.
  4. Appropriate functions for locking access have been added so that the CSndQueue methods can make signal and waiting through the CSndUList object.

Fixed problems:

  1. Locking wasn't applied on some methods of CSndUList while the internal data were used by other threads (notably CSndUList::m_iLastEntry, which should be updated in sync with other fields in this class; the so-far solution likely stated that this field can be read atomically, but in reality this was completely unnecessary if a lock needs to be applied anyway in order not to miss a signal on CV).
  2. Separate m_ListLock and m_WindowLock were unnecessary because the first lock locks the access to the data of CSndUList and m_WindowLock is a lock for CV that should signal an important update of the internal data (new data have been added to the list). Using separate locks for these purposes only increased the possibility to miss the update signal.

Likely replaced by #1844

@ethouris
Copy link
Collaborator Author

Tracing the possible call paths:

  1. The insert_norealloc_ can be called from insert_ that is called from update. This is usually called in the main thread
  2. The insert_norealloc_ can be called from pop that is called in the receiver worker thread

In the 2nd case this is called in the same thread that would do wait, so any update here will not be notified, that is, the notification doesn't have a target to reach. I think this shouldn't be a problem.

In the 1st case it's a typical CV information between threads. The receiver worker thread picks up new entries in the loop, and when the list has ended, it enters the waiting. The locks for pickup and waiting are separate critical sections, but this should be a problem because the receiver worker thread is the only thread that removes entries, so it's not possible that an element has been removed between checking existence in the waiting loop and trying to pick it up from the list.

@ethouris
Copy link
Collaborator Author

TEST: Made a short transmission tests with short pauses (to stall transmission for a while), no problems observed.

@maxsharabayko maxsharabayko added this to the v1.4.4 milestone Jun 21, 2021
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code labels Jun 21, 2021
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.

2 participants