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

(Suggestion) Background Process for Checking Crashes / Other LMMS Background Processes #4408

Closed
peefTube opened this issue Jun 6, 2018 · 6 comments · Fixed by #6366
Closed
Assignees

Comments

@peefTube
Copy link

peefTube commented Jun 6, 2018

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:

  1. If some sort of fatal error occurs that cannot be handled without extremely convoluted code, LMMS will crash, likely to desktop. In this scenario it makes sense to keep this suggested process external, so whatever is going on with LMMS does not affect the process itself. It also saves a lot of room within LMMS' code.
  2. A smaller process will take up relatively little RAM/CPU compared to a bigger program like LMMS, and if it is separate from the instances of LMMS the RAM/CPU usage remains constant rather than increasing per instance (since only one instance is allowed per the suggestion).

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.

@Wallacoloo
Copy link
Member

Wallacoloo commented Jun 6, 2018

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 ProcessWatcher class in include/RemotePlugin.h. Here's some code to check if a process's parent is still alive, just meant as reference: https://forum.qt.io/topic/33964/solved-child-qprocess-that-dies-with-parent/8

But as long as we can control what code runs in the child process, I don't think we need any additional processes.

@peefTube
Copy link
Author

peefTube commented Jun 6, 2018

Yeah, I was thinking a parent-child hierarchy would be useful, lol.

@lukas-w
Copy link
Member

lukas-w commented Jun 6, 2018

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.

@PhysSong
Copy link
Member

PhysSong commented Jun 6, 2018

Remote plugins use sockets on POSIX system. Thus, RemotePluginBase::read will invalidate RemotePluginClient instances when LMMS crashes.
Alternatively, we may implement something like ProcessWatcher. It will require linking remote plugins against Qt libraries.

@DomClark
Copy link
Member

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 RemoteZynAddSubFx can get left running too if LMMS dies.

@Spekular Spekular mentioned this issue Mar 31, 2020
35 tasks
@Spekular
Copy link
Member

Closing and consolidating into #5433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants