-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Fix deadlock between nvWave stop and feed #11886
Conversation
with self._global_waveout_lock: | ||
winmm.waveOutPrepareHeader(self._waveout, LPWAVEHDR(whdr), sizeof(WAVEHDR)) | ||
winmm.waveOutWrite(self._waveout, LPWAVEHDR(whdr), sizeof(WAVEHDR)) | ||
self.sync() # sync must still be called even if stopping, so that waveOutUnprepareHeader can be called | ||
self._prev_whdr = whdr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bug, in the case that nvWave
is stopping, we shouldn't save this unprepared header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what then would unprepare that header? saving it off means that feed / sync will unprepare it next time (I think) not saving it, and setting _prev_whdr to None, you'd have to specifically call waveOutUnprepare for hdr I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
The base branch was changed.
This should be in 2020.4. @michaelDCurran are you still happy with this? |
@feerrenrut I'm happy for this to be in 2020.4, yes. |
Link to issue number:
fixes #11591
Summary of the issue:
On at least one user's system, a deadlock seems to occur while using NVDA with the oneCore synth. This results in a freeze and a stack dump by the watchdog.
Stacks to note, from log:
Because I can't reproduce this, I'm having to reason about how the code got into this state. What seems to have happened is that
feed
andstop
have been called at the same time from the synth thread and the main thread respectively. These have both progressed to sub-routines,sync
and_idleUnbuffered
respectively._idleUnbuffered
is blocked waiting to acquire theself._lock
, andsync
has acquired theself._lock
mutex and is blocked bywaitForSingleObject
.feed
has no locks until it calls into_feedUnbuffered
, howeverstop
does acquire locks that_feedUnbuffered
needs, then releases them and then calls to_idleUnbuffered
which has contention with_feedUnbuffered
again. It seems to me that the only way to get into this situation is ifstop
acquires the locks it needs first. It callswaveOutReset
, then when it releases the locks and before they are acquired by_idleUnbuffered
they are instead acquired by_feedUnbuffered
. This results inwaveOutWrite
being called before progressing tosync
and waiting onwaitForSingleObject
It's still not entirely clear to me why this set of events causes a deadlock. My best guess is that on this particular system, the
whdr
(Wave Out Header) which includesdwFlags
that should have theWHDR_DONE
bit set when the associated buffer has finished being played is not getting updated due to the call to reset. Thus the synth thread gets stuck in a loop callingwaitForSingleObject
and checking_prev_whdr
.Alternatively, it may be that the stack is not representative of the freeze?
Description of how this pull request fixes the issue:
Because I'm not 100% sure of the cause of the problem, I have covered a few potentials:
waitForSingleObject
now has a timeout, it then will check ifnvWave
has been stopped before waiting again.winm
isn't explicit about whether this is handled.Relevant Info from Docs:
When opening
winm
device, the event signal args:waveOutReset:
The locking logic for
nvWave
is complicated, however given plans to re writenvWave
with modern Windows API's I think we should hold off on doing more than we need to resolve this specific issueTesting performed:
Ran locally with oneCore
Known issues with pull request:
None
Change log entry:
None