REFAC(client, audion): Various refactorings in AudioOutput #6708
Conversation
If QMultiHash::value() was called at the exact same time that other parts of the code mutate the map, this could lead to a data race. By protecting the map access via the associated lock, concurrent modification is prevented (assuming all mutating code obtain a write lock as they should).
This is now encoded into the type system as it is illegal to dereference a void *. Previously, calling functions just needed to know that the passed pointer was never going to be dereferenced (in order to determined whether or not the function call may happen while not holding the qrwlOutputs lock.
The AudioOutput::removeUser function used to initiate a signal being emitted that leads to the deletion of the audio buffer related to the given user and also to the removal of that user (and buffer) from the qmOutputs map. Since removeUser has (thus far) always been called from the main thread (in which the AudioOutput QThread object lives), the connection has been direct, meaning that once removeUser returns to the caller, the user has indeed been removed from AudioOutput. However, if removeUser was called from a different thread, the connection would be queued, meaning that AudioOutput still holds a reference (or rather pointer) to that user when removeUser returns. This can easily lead to a memory corruption issue. Therefore, this commit ensures that removeUser always makes a direct function call to remove all references to that user so that the caller can be sure that AudioOutput will have forgotten about that particular user once that function returns.
Before, the actual wiping of elements was delegated to the removeBuffer function by means of Qt's signal/slot mechanism. This required a rather inefficient implementation inside wipe() (using an auxiliary list), lead to constant locking and re-locking (since a write lock is acquired and released for every buffer individually) and made it unclear whether after AudioOutput::wipe() returns, the buffer has actually been wiped (due to the way the Auto connection type for Qt signal/slots work). This commit ensures that wipe no longer delegates the work, thereby fixing all the abovementioned issues.
AudioOutput::addFrameToBuffer used to emit a signal for deleting the old speech buffer. However, it implicitly relied on that connection being direct (i.e. it relied on addFrameToBuffer to be called from the main thread) as otherwise, there would be the potential for a memory leak where the speech buffer is not processed by removeBuffer before the addFrameToBuffer call has replaced the associated entry in qmOutputs. At that point speech will be the last reference pointing to that object, and as soon as speech goes out of scope, the memory for that object would have leaked. In order to make the control flow more clear, we now skip the signal emission and instead call removeBuffer directly. This has been done in a way that also prevents the need for an additional acquisition of a write lock.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@Hartmnt I just went through our discussion in #5926 again and I think the entire business of double-deletion was wrong because the |
There was a problem hiding this comment.
@Krzmbrzl I tried to read through our old conversation, but I am not able to follow all the thinking from back then. I approve refactoring the AudioOutput logic anyway 😄
This then probably also addresses #6613 right?
What are your thoughts on back-porting this? I tend towards "too big and too prone to introduce new problems"
| } | ||
|
|
||
| // We rely on removeBuffer not actually dereferencing the passed pointer. | ||
| // It it did, releasing the lock before calling the function cries for trouble. |
There was a problem hiding this comment.
| // It it did, releasing the lock before calling the function cries for trouble. | |
| // If it did, releasing the lock before calling the function cries for trouble. |
| @@ -1031,7 +1031,8 @@ void AudioInput::encodeAudioFrame(AudioChunk chunk) { | |||
|
|
|||
| if (stopActiveCue) { | |||
| // Cancel active cue first, if there is any | |||
| ao->removeToken(m_activeAudioCue); | |||
| ao->invalidateToken(m_activeAudioCue); | |||
| m_activeAudioCue = {}; | |||
There was a problem hiding this comment.
What advantage has this change? Wouldn't we rely on future code to always make sure to set their token to {} when calling invalidateToken?
There was a problem hiding this comment.
The advantage is that the houskeeping of the token should be the responsibility of the owner. If non-owning code starts to do delete-like functionality, things can get very messy very quickly.
| // that attempting to lock for read access will fail, whereas it | ||
| // would work if the calling scope only held a read lock). | ||
| assert(acquireWriteLock || !qrwlOutputs.tryLockForRead(0)); | ||
| QWriteLocker locker(acquireWriteLock ? &qrwlOutputs : nullptr); |
There was a problem hiding this comment.
Without reading up on the documentation myself, acquiring a lock on nullptr is defined and a no-op?
There was a problem hiding this comment.
Yes, that's what the docs state. I double-checked that myself as well :D
Unfortunately, I don't think it does. These really are refactorings that leave the actual behavior (mostly) the same. And given that the only fix in here is the potential data race on map access, which I would consider to ne very unlikely in the first place, I would tend to not backport this. |
All changes are related to making the control flow more explicit and also more deterministic than it currently is with relying on Qt's signal/slot mechanism for handling buffer deletions. This is important as sometimes, a given buffer really needs to be removed now and not just sometime in the future.
The current code achieves this because in the specific context that the relevant functions are called, the connection that Qt will make are direct and therefore no asynchronous operations take place. But this is absolutely not obvious from the current code. It should, however, be with the refactored code.
Checks