-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
(Suggestion) Background Process for Checking Crashes / Other LMMS Background Processes #4408
Comments
So the issue is that plugin processes are outliving LMMS whenever LMMS crashes? I have very limited knowledge in this area of LMMS and in the operating system process model in general. However, I expect we must instantiate our own process for each VST plugin, and that we can control what code that process runs. If that's the case, the very simplest answer is for the child to instruct the operating system to kill it when its parent dies: https://stackoverflow.com/a/284443/216292 Of course, something this straightforward only works on Linux. In general, the more cross-platform approach seems to be for the child process to regularly query the status of its parent, and then exit when it detects that its parent has also exited. Each child process could put this logic in a dedicated thread, if you like, so that it's resistant to deadlocks or other logical errors. It's almost the reverse of our But as long as we can control what code runs in the child process, I don't think we need any additional processes. |
Yeah, I was thinking a parent-child hierarchy would be useful, lol. |
I implemented this some time ago for Unix, but forgot to push apparently. Here's the diff: diff --git a/plugins/vst_base/RemoteVstPlugin.cpp b/plugins/vst_base/RemoteVstPlugin.cpp
index 04ff97731..8aad8bc62 100644
--- a/plugins/vst_base/RemoteVstPlugin.cpp
+++ b/plugins/vst_base/RemoteVstPlugin.cpp
@@ -56,6 +56,12 @@
#include <sched.h>
#endif
+#if defined(LMMS_BUILD_APPLE) || defined(LMMS_BUILD_LINUX)
+#include <sys/signal.h>
+#include <sys/types.h>
+#include <unistd.h>
+#endif
+
#include <wine/exception.h>
#endif
@@ -63,6 +69,7 @@
#define USE_WS_PREFIX
#include <windows.h>
+#include <thread>
#include <vector>
#include <queue>
#include <string>
@@ -1972,6 +1979,20 @@ int main( int _argc, char * * _argv )
return -1;
}
+ bool running = true;
+#if defined(LMMS_BUILD_APPLE) || defined(LMMS_BUILD_LINUX)
+ // End this process when parent dies
+ std::thread poll_parent_thread([&running]() {
+ while (running) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(500));
+ if (getppid() == 1) {
+ kill(getpid(), SIGHUP);
+ break;
+ }
+ }
+ });
+#endif
+
#ifdef LMMS_BUILD_WIN32
#ifndef __WINPTHREADS_VERSION
// (non-portable) initialization of statically linked pthread library
@@ -2087,6 +2108,9 @@ int main( int _argc, char * * _argv )
delete __plugin;
+ running = false;
+ poll_parent_thread.join();
+
#ifdef LMMS_BUILD_WIN32
#ifndef __WINPTHREADS_VERSION I can push and open a PR if desired. |
Remote plugins use sockets on POSIX system. Thus, |
On Windows the normal way to do this would be to use job objects. Here's a diff implementing this: diff --git a/src/core/RemotePlugin.cpp b/src/core/RemotePlugin.cpp
index d0eafbfa3..7939baf87 100644
--- a/src/core/RemotePlugin.cpp
+++ b/src/core/RemotePlugin.cpp
@@ -41,6 +41,11 @@
#include <sys/un.h>
#endif
+#ifdef LMMS_BUILD_WIN32
+#include <windows.h>
+
+HANDLE s_job = NULL;
+#endif
// simple helper thread monitoring our RemotePlugin - if process terminates
// unexpectedly invalidate plugin so LMMS doesn't lock up
@@ -200,6 +205,18 @@ bool RemotePlugin::init(const QString &pluginExecutable,
return failed();
}
+#ifdef LMMS_BUILD_WIN32
+ if( !s_job )
+ {
+ s_job = CreateJobObjectA( NULL, NULL );
+ JOBOBJECT_EXTENDED_LIMIT_INFORMATION limitInfo;
+ limitInfo.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+ SetInformationJobObject( s_job, JobObjectExtendedLimitInformation,
+ &limitInfo, sizeof( JOBOBJECT_EXTENDED_LIMIT_INFORMATION ) );
+
+ }
+#endif
+
QStringList args;
#ifdef SYNC_WITH_SHM_FIFO
// swap in and out for bidirectional communication
@@ -213,6 +230,9 @@ bool RemotePlugin::init(const QString &pluginExecutable,
m_process.setProcessChannelMode( QProcess::ForwardedChannels );
m_process.setWorkingDirectory( QCoreApplication::applicationDirPath() );
m_process.start( exec, args );
+#ifdef LMMS_BUILD_WIN32
+ AssignProcessToJobObject( s_job, m_process.pid()->hProcess );
+#endif
m_watcher.start( QThread::LowestPriority );
#else
qDebug() << exec << args;
Once LMMS exits, normally or abnormally, Windows closes the job handle and subsequently kills any left over remote processes. Is this something we want in 1.2, or in master? We also ought not to forget that |
Closing and consolidating into #5433 |
I am certain the topic of how to deal with VST processes after an LMMS crash through programming has gone through the motions in this GitHub repository before. Even if not, I'd like to bring it to the developers' attention with a potential solution of my own.
The idea is to create a much smaller, background process (maybe a small .dll on Windows, or something very similar, either way it should run as a process) contained within the LMMS directory started up by LMMS.exe (with a check in LMMS to see if the process is running, if so, the process will not start up again). Once this process is started, it should check the running processes to see if LMMS.exe is running first.
If LMMS (any instance of LMMS.exe) is running:
A value should be set to true to ignore the checks on things like VST processes, etc. that are being used
The program should continue the loop of checking LMMS, preferably on an interval between 5 to 20 seconds so it's not hogging RAM/CPU but also not allowing stray VSTs to hog RAM/CPU either
BUT if LMMS is NOT running:
This value should be set to false immediately and all pertinent processes should be shut down before the process itself shuts itself down.
There's two reasons I say this should be an external process:
This is a fairly simple solution to the problem, considering the nature of the problem and the scope of potential scenarios which the solution covers. It is also a preemptive strike against possible crash-errors in the future of LMMS' development that may not be able to be handled properly, leading to stray VSTs. It may seem like overkill, and ultimately is a suggestion, but I urge the developers to look into this as a potential solve.
Thank you for your time.
EDIT: I didn't do such a great job implying this, so I'll just clarify: when the process shuts down the VSTs, it should be telling either the VST processes OR the OS to close the processes down. The latter is preferable if VST processes have a chance of becoming unresponsive after LMMS crashes.
The text was updated successfully, but these errors were encountered: