QProcess cleanup #1076

Try to softly shutdown QProcess instances or allow the QProcess destructor to SIGKILL.

    The major change are the calls to setparent that result in the underlying qprocess being removed by the destructor (killing the nvim process).
    For the GUI close event we first try to wait 500ms for the process to terminate (this may have to be tuned).
    Tests were adjusted to make this a bit easier to manage with a QSharedPointer, the main reason for this was the issues we see in windows with leftover process breaking CI.

There is still some flakyness in the tests, with occasional failures, but at least tests succeed more often now. No doubt that the root cause for races in tests is still present.

Given the issues we saw with CI, I have removed all github CI windows. Troubleshooting them was always hard and some tests systematically failed.

.github/workflows/build-test.yml

@@ -49,30 +49,6 @@ jobs:
             cc: clang
             cxx: clang++
             publish: true
-
-          # Qt 5.15 only supports MinGW 8.11. The `windows-latest` and `windows-2022`
-          # images bundle only 11.2. For now, we must use the older image `windows-2019`.
-          - name: Windows_MingGW
-            flavor: Debug
-            runner: windows-2022
-            generator: MinGW Makefiles
-            qtver: 5.15.2
-            qtdir: mingw81_64
-            qtstr: windows desktop win64_mingw81
-            cc: gcc
-            cxx: g++
-            ctest_exe_args: -tap
-
-          - name: Windows_Release
-            flavor: Release
-            runner: windows-2022
-            generator: Visual Studio 17 2022
-            qtver: 5.15.2
-            qtdir: msvc2019_64
-            qtstr: windows desktop win64_msvc2019_64
-            ctest_exe_args: -tap
-            publish: true
-
     runs-on: ${{ matrix.runner }}
     steps:
       - uses: actions/checkout@v2
@@ -96,33 +72,6 @@ jobs:
           brew ls --formula | grep -wq msgpack || brew install msgpack
           brew link qt5 --force
 
-      - name: Windows - Setup
-        if: ${{ startsWith(matrix.runner, 'windows') }}
-        env:
-          QT_DIR: ${{ github.workspace }}\${{ matrix.qtver }}\${{ matrix.qtdir }}
-        run: |
-          New-Item -Path .\build -Name "build" -ItemType "directory"
-          Invoke-WebRequest https://github.com/neovim/neovim/releases/download/stable/nvim-win64.zip -OutFile nvim-win64.zip
-          Expand-Archive -Path nvim-win64.zip -DestinationPath .\build\
-          Add-Content -Path $env:GITHUB_PATH -Value ${{ github.workspace }}\build\nvim-win64\bin\
-          Add-Content -Path $env:GITHUB_ENV -Value "CMAKE_PREFIX_PATH=$env:QT_DIR;$env:QT_DIR\lib\cmake"
-          Add-Content -Path $env:GITHUB_PATH -Value "${{ env.qt_dir }}"
-          Add-Content -Path $env:GITHUB_PATH -Value "${{ env.qt_dir }}\bin"
-
-      - name: Qt Cache Restore
-        if: ${{ matrix.qtver }}
-        id: cache-qt
-        uses: actions/cache@v2
-        with:
-          path: ${{ matrix.qtver }}\${{ matrix.qtdir }}
-          key: qt-${{ runner.os }}-${{ matrix.qtver }}-${{ matrix.qtdir }}
-
-      - name: Qt Cache Install
-        if: ${{ matrix.qtver && steps.cache-qt.outputs.cache-hit != 'true' }}
-        run: |
-          pip install aqtinstall
-          python -m aqt install ${{ matrix.qtver }} ${{ matrix.qtstr }}
-
       #
       # Build and Test
       #
@@ -169,16 +118,6 @@ jobs:
           macdeployqt ./build/bin/nvim-qt.app -dmg
           mv ./build/bin/nvim-qt.dmg neovim-qt.dmg
 
-      - name: Windows - Publish
-        if: ${{ matrix.publish && startsWith(matrix.runner, 'windows') }}
-        run: |
-          cmake --build ./build --target install
-          Push-Location ${{ github.workspace }}/build
-          cpack --verbose -G WIX
-          Pop-Location
-          Compress-Archive -Path ./install -DestinationPath neovim-qt.zip
-          Move-Item -Path ./build/neovim-qt-installer.msi -Destination neovim-qt-installer.msi
-
       - name: Upload Artifacts
         if: ${{ matrix.publish }}
         uses: actions/upload-artifact@v2

contrib/appveyor.yml

@@ -10,16 +10,19 @@ environment:
     GENERATOR: MinGW Makefiles
     RELEASE_ARTIFACT: true
     CMAKE_PATH_PREFIX: C:\Qt\5.15\mingw81_64\lib\cmake
+    CMAKE_BUILD_TYPE: Release
   - PlatformToolset: VisualStudio2019
     QTPATH: C:\Qt\5.15\msvc2019_64
     GENERATOR: Visual Studio 16
     CMAKE_PATH_PREFIX: C:\Qt\5.15\msvc2019_64\lib\cmake
+    CMAKE_BUILD_TYPE: Debug
   - PlatformToolset: MinGW-Win64-Qt6
     QTPATH: C:\Qt\6.2.4\mingw_64
     GENERATOR: MinGW Makefiles
     RELEASE_ARTIFACT: false
     CMAKE_PATH_PREFIX: C:\Qt\6.2.4\mingw_64\lib\cmake
     EXTRA_CMAKE_ARGS: -DWITH_QT=Qt6
+    CMAKE_BUILD_TYPE: Release
 configuration:
 - RelWithDebInfo
 matrix:
@@ -42,7 +45,7 @@ build_script:
 - echo %PATH%
 - mkdir build
 - cd build
-- cmake -G "%GENERATOR%" -DCMAKE_PREFIX_PATH="%CMAKE_PATH_PREFIX%" -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=../INSTALL -DENABLE_TESTS=ON %EXTRA_CMAKE_ARGS% ..
+- cmake -G "%GENERATOR%" -DCMAKE_PREFIX_PATH="%CMAKE_PATH_PREFIX%" -DCMAKE_BUILD_TYPE=%CMAKE_BUILD_TYPE% -DCMAKE_INSTALL_PREFIX=../INSTALL -DENABLE_TESTS=ON %EXTRA_CMAKE_ARGS% ..
 - cmake --build . --config Release --target install
 - cpack --verbose -G WIX
 test_script:

src/gui/mainwindow.cpp

@@ -1,6 +1,7 @@
 #include "mainwindow.h"
 
 #include <QCloseEvent>
+#include <QEventLoop>
 #include <QLayout>
 #include <QSettings>
 #include <QStyleFactory>
@@ -42,11 +43,11 @@ void MainWindow::init(NeovimConnector *c)
 	}
 
 	m_shell = new Shell(c);
+	m_shell->setParent(this);
 
 	addToolBar(&m_tabline);
 
 	m_nvim = c;
-	m_nvim->setParent(this);
 
 	// GuiShowContextMenu - right click context menu and actions.
 	m_contextMenu = new ContextMenu(c, this);
@@ -235,6 +236,18 @@ void MainWindow::neovimGuiCloseRequest(int status)
 {
 	m_neovim_requested_close = true;
 	m_exitStatus = status;
+
+	// Try to wait for neovim to quit
+	QTimer timer;
+	timer.setSingleShot(true);
+	QEventLoop loop;
+	connect(m_nvim, &NeovimConnector::processExited, &loop, &QEventLoop::quit);
+	connect(m_nvim, &NeovimConnector::aboutToClose, &loop, &QEventLoop::quit);
+	timer.start(500);
+	loop.exec();
+	bool timed_out = !timer.isActive();
+	qDebug() << "Waited for neovim close, timed out:" << timed_out;
+
 	QMainWindow::close();
 	m_neovim_requested_close = false;
 }

src/gui/shell.cpp

@@ -92,6 +92,8 @@ Shell::Shell(NeovimConnector *nvim, QWidget *parent)
 		return;
 	}
 
+	m_nvim->setParent(this);
+
 	connect(m_nvim, &NeovimConnector::error,
 			this, &Shell::neovimError);
 	connect(m_nvim, &NeovimConnector::processExited,

src/gui/shell.h

@@ -70,6 +70,7 @@ class Shell: public ShellWidget
 	static void DispatchRedrawNotifications(
 		T* pThis, const QVariantList& args) noexcept;
 
+	NeovimConnector* nvim() { return m_nvim; }
 signals:
 	void neovimTitleChanged(const QString &title);
 	void neovimBusyChanged(bool);

src/msgpackiodevice.cpp

@@ -60,6 +60,7 @@ MsgpackIODevice::MsgpackIODevice(QIODevice* dev, QObject* parent)
 		m_dev->setParent(this);
 		connect(m_dev, &QAbstractSocket::readyRead,
 				this, &MsgpackIODevice::dataAvailable);
+		connect(m_dev, &QIODevice::aboutToClose, this, &MsgpackIODevice::aboutToClose);
 
 		if ( !m_dev->isSequential() ) {
 			setError(InvalidDevice, tr("IO device needs to be sequential"));

src/msgpackiodevice.h

@@ -71,6 +71,7 @@ class MsgpackIODevice: public QObject
 	/** A notification with the given name and arguments was received */
 	void notification(const QByteArray &name, const QVariantList& args);
 	void encodingChanged(const QByteArray& encoding);
+	void aboutToClose();
 
 protected:
 	void sendError(const msgpack_object& req, const QString& msg);

src/neovimconnector.cpp

@@ -37,6 +37,9 @@ NeovimConnector::NeovimConnector(MsgpackIODevice *dev)
 
 	connect(m_dev, &MsgpackIODevice::error,
 			this, &NeovimConnector::msgpackError);
+	connect(m_dev, &MsgpackIODevice::aboutToClose, this, &NeovimConnector::aboutToClose);
+
+	m_dev->setParent(this);
 
 	if ( !m_dev->isOpen() ) {
 		return;

src/neovimconnector.h

@@ -106,6 +106,7 @@ class NeovimConnector: public QObject
 	void ready();
 	void error(NeovimQt::NeovimConnector::NeovimError);
 	void processExited(int exitCode);
+	void aboutToClose();
 
 public slots:
 	void fatalTimeout();

test/common_gui.cpp

@@ -35,7 +35,7 @@ template<class T> static void ValidateNeovimConnection(T* obj) noexcept
 	Q_ASSERT(obj->isNeovimAttached());
 }
 
-std::pair<NeovimConnector*, Shell*> CreateShellWidget() noexcept
+QSharedPointer<Shell> CreateShellWidget() noexcept
 {
 	DisableLocalGInitVim();
 	NeovimConnector* c{ NeovimConnector::spawn(cs_argsNone) };
@@ -45,10 +45,10 @@ std::pair<NeovimConnector*, Shell*> CreateShellWidget() noexcept
 
 	ValidateNeovimConnection(s);
 
-	return { c, s };
+	return QSharedPointer<Shell>(s);
 }
 
-std::pair<NeovimConnector*, MainWindow*> CreateMainWindow() noexcept
+QSharedPointer<MainWindow> CreateMainWindow() noexcept
 {
 	NeovimConnector* c{ NeovimConnector::spawn(cs_argsNone) };
 	MainWindow* w{ new MainWindow{ c } };
@@ -57,10 +57,10 @@ std::pair<NeovimConnector*, MainWindow*> CreateMainWindow() noexcept
 
 	ValidateNeovimConnection(w);
 
-	return { c, w };
+	return QSharedPointer<MainWindow>(w);
 }
 
-std::pair<NeovimConnector*, MainWindow*> CreateMainWindowWithRuntime() noexcept
+QSharedPointer<MainWindow> CreateMainWindowWithRuntime() noexcept
 {
 	static const QStringList cs_argsNoneRuntime{
 		"-u", "NONE", "--cmd", "set rtp+=" + GetRuntimeAbsolutePath()
@@ -74,7 +74,7 @@ std::pair<NeovimConnector*, MainWindow*> CreateMainWindowWithRuntime() noexcept
 
 	ValidateNeovimConnection(w);
 
-	return { c, w };
+	return QSharedPointer<MainWindow>(w);
 }
 
 } // namespace NeovimQt

test/common_gui.h

@@ -1,16 +1,17 @@
 #pragma once
 
+#include <QSharedPointer>
 #include <gui/mainwindow.h>
 #include <gui/shell.h>
 #include <neovimconnector.h>
 #include <utility>
 
 namespace NeovimQt {
 
-std::pair<NeovimConnector*, Shell*> CreateShellWidget() noexcept;
+QSharedPointer<Shell> CreateShellWidget() noexcept;
 
-std::pair<NeovimConnector*, MainWindow*> CreateMainWindow() noexcept;
+QSharedPointer<MainWindow> CreateMainWindow() noexcept;
 
-std::pair<NeovimConnector*, MainWindow*> CreateMainWindowWithRuntime() noexcept;
+QSharedPointer<MainWindow> CreateMainWindowWithRuntime() noexcept;
 
 } // namespace NeovimQt

test/tst_neovimconnector.cpp

@@ -20,6 +20,7 @@ private slots:
 		QCOMPARE(c.canReconnect(), false);
 
 		NeovimConnector *spawned = NeovimConnector::spawn({"-u", "NONE"});
+		spawned->setParent(this);
 		QCOMPARE(spawned->connectionType(), NeovimConnector::SpawnedConnection);
 		QCOMPARE(spawned->canReconnect(), true);
 
@@ -29,6 +30,7 @@ private slots:
 	void isReady() {
 
 		NeovimConnector *c = NeovimConnector::spawn({"-u", "NONE"});
+		c->setParent(this);
 		QSignalSpy onReady(c, SIGNAL(ready()));
 		QVERIFY(onReady.isValid());
 
@@ -38,7 +40,7 @@ private slots:
 
 	void encodeDecode() {
 		NeovimConnector *c = NeovimConnector::spawn({"-u", "NONE"});
-
+		c->setParent(this);
 		// This will print a warning, but should succeed
 		QString s = "ç日本語";
 		QByteArray bytes = c->encode(s);
@@ -55,6 +57,7 @@ private slots:
 
 	void connectToNeovimTCP() {
 		NeovimConnector *c = NeovimConnector::connectToNeovim("127.0.0.1:64999");
+		c->setParent(this);
 		QCOMPARE(c->connectionType(), NeovimConnector::HostConnection);
 		QSignalSpy onError(c, SIGNAL(error(NeovimError)));
 		QVERIFY(onError.isValid());
@@ -68,6 +71,7 @@ private slots:
 
 	void connectToNeovimSocket() {
 		NeovimConnector *c = NeovimConnector::connectToNeovim("NoSuchFile");
+		c->setParent(this);
 		QCOMPARE(c->connectionType(), NeovimConnector::SocketConnection);
 		QSignalSpy onError(c, SIGNAL(error(NeovimError)));
 		QVERIFY(onError.isValid());
@@ -82,6 +86,7 @@ private slots:
 	void connectToNeovimEnvEmpty() {
 		// This is the same as ::spawn()
 		NeovimConnector *c = NeovimConnector::connectToNeovim("");
+		c->setParent(this);
 		QSignalSpy onReady(c, SIGNAL(ready()));
 		QVERIFY(onReady.isValid());
 		QVERIFY(SPYWAIT(onReady));