Prevent lmms freezing when a vst hangs #4790
Conversation
* use timed mutexes to avoid hangs * removed HWNDPARENT call * terminate process when invalidating * 32 bit remote process install location change
|
|
||
| QProcess m_process; | ||
| ProcessWatcher m_watcher; | ||
|
|
||
| QMutex m_commMutex; | ||
| std::recursive_timed_mutex m_commMutex; |
There was a problem hiding this comment.
What's the reason you changed to std::recursive_timed_mutex? I think we can keep using QMutex, avoiding the MinGW compatibility problems.
There was a problem hiding this comment.
I would like to avoid QT at as much as possible when it's not related to the UI. In the longer term it would, hopefully, make it easier to separate lmms into two parts: a library and what it is now.
If it's not possible to not use QMutex for this, I'll of course change it.
There was a problem hiding this comment.
I understand wanting to avoid Qt for core functionality. I'm just not very happy about the MinGW mutex support detection & workaround and would like to avoid it being copy&pasted around the CMake scripts.
What do you think about reverting this PR to QMutex for now until (a) we find a cleaner solution or (b) we drop support for old MinGW versions? An example of how (a) could look like can be seen in 57ca681 which is part of #4443. In this commit, I added include/mingw-std-threads/mutex which shadows the system mutex using include_directories(BEFORE include/mingw-std-threads) when it's found necessary. If done correctly, this could fix MinGW thread support for all targets in the project without the need of #ifdef USE_MINGW_THREADS_REPLACEMENT checks at every include.
There was a problem hiding this comment.
Give me a couple of days to think this over... I've deleted my previous msg because I responded to quickly; there may be other options.
Are we using mingw for any other reason than to cross-compile to windows?
This pr won't be merged any time soon, so we have all the time we need to find a solution that's clean and acceptable.
There was a problem hiding this comment.
@justnope Don't worry about using QtCore functionality inside our core, this is completely fine and we'll probably always do it. It doesn't create any coupling between core and UI. The only reason I can see to use std mutex here is re-usability outside of LMMS. However, If you ever need to use this code outside of LMMS, you can simply swap QMutex for std::recursive_timed_mutex, their APIs are compatible.
Are we using mingw for any other reason than to cross-compile to windows?
We use winegcc for RemoteVstPlugin, and we support MSYS2 on Windows which uses MinGW.
| lock(); | ||
| sendMessage( message( IdVstSetTempo ).addInt( _bpm ) ); | ||
| unlock(); | ||
| if (try_lock()) |
There was a problem hiding this comment.
Can we invert the control flow here and at the other places where try_lock is used to this:
if (! try_lock())
return;
}I think it's more natural this way because it doesn't have the normal case inside an if condition and as a bonus we avoid all the indentation changes, which makes reviewing easier.
| SetWindowLongPtr( (HWND)(intptr_t) m_pluginWindowID, | ||
| GWLP_HWNDPARENT, | ||
| (LONG_PTR) gui->mainWindow()->winId() ); | ||
| //SetWindowLongPtr( (HWND)(intptr_t) m_pluginWindowID, |
There was a problem hiding this comment.
From my point of view:
It's complicated.
When you do stuff like this, it solves some problems but also creates others. What this does, short version, it links processes together in a single message queue and therefor when one hangs, the other one does too.
I think this was the first version because something similar happens when the user selects "plugging embedding". I deleted it so nothing happens when they select "no embedding" and normally the same things happen when they select qt or win32 embedding.
There was a problem hiding this comment.
This is here to make plugin UIs always-on-top when not embedded. It does indeed attach the input queues, but that shouldn't cause any extra problems since the queues are attached in embed mode anyway. Also, plugins are generally designed to have the same input queue as the host, since most hosts load the plugin into the host's own process, so this isn't exactly a bad idea.
ghost
closed this
|
What's the reason for closing this @justnope? In my eyes this PR is a valuable contribution, I'd very much like to see it merged. |
|
It's quicker to start from scratch than to revert everything. |
|
@lukas-w I don't know what to do. Recap: This isn't a fix, this is a work-around. When @DomClark said it isn't a bad idea to have the input queues linked together, it actually made this work-around invalid. I explained it in discord and they agreed. If they already forgot what makes this work then maybe we should rethink the approach. Long story short: A thread blocks because a vst blocks. Also: lmms transmits using win msg queues keep alives. I've added timeouts to detect blocks (in the msg queue). If it timeouts, we kill the process and whatever. This results in also unblocking the thread waiting for a reply. And presto, lmms doesn't freeze. If Dom already forgot what the key point of this fix is, then maybe we should rethink it. QCore doesn't provide a timeout for what we use to communicate across processes. Therefor, I used this helter-skelter fix/work-around. It's easy to reproduce. Attach a debugger to lmms, attach one to the vst process. In the debugger, hang the vst process; lmms should recover.
|
I've added a detection to see if a VST hangs using timed mutexes. When a VST hangs it will block the msg semaphore, but lmms still sends an idleUpdate. That idleUpdate will also block and call invalidate on the plugin.
I've added m_process.kill in case the plugin is embedded. This is necessary because at that time the windows input queues are linked and one can block the other.