[education/rkward] /: Fix lockup condition while cancelling commands (expecting to pass all tests, on Windows, again)

[Thomas Friedrichsmeier]

Git commit 2b10af556e5c72b61a53f81a8a15e7699148d15a by Thomas Friedrichsmeier.
Committed on 02/10/2022 at 11:32.
Pushed by tfry into branch 'master'.

Fix lockup condition while cancelling commands (expecting to pass all tests, on Windows, again)

M  +1    -1    .kde-ci.yml
M  +1    -0    ChangeLog
M  +28   -2    rkward/autotests/core_test.cpp
M  +6    -3    rkward/rbackend/rkrinterface.cpp

https://invent.kde.org/education/rkward/commit/2b10af556e5c72b61a53f81a8a15e7699148d15a

diff --git a/.kde-ci.yml b/.kde-ci.yml
index 0461eeb4..9c9d31aa 100644
--- a/.kde-ci.yml
+++ b/.kde-ci.yml
@@ -19,6 +19,6 @@ Dependencies:
     'frameworks/kcrash': '@stable'
 
 Options:
-  require-passing-tests-on: [ 'Linux', 'FreeBSD' ]
+  require-passing-tests-on: [ 'Linux', 'FreeBSD', 'Windows' ]
   tests-load-sensitve: True
   per-test-timeout: 120
diff --git a/ChangeLog b/ChangeLog
index c4794254..e1e29e54 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,4 @@
+- Fixed: In corner cases, cancelling commands could lead to a lockup
 - Fixed: IRT Cronbach's Alpha did not work for subsets, if the data.frame name contains dots
 - Fixed: Action to remove several rows in data editor, simultaneously, always remained disabled
 - Fixed: Workspace browser would not always show change, immediately, when object type changes
diff --git a/rkward/autotests/core_test.cpp b/rkward/autotests/core_test.cpp
index b2541f45..9d79b894 100644
--- a/rkward/autotests/core_test.cpp
+++ b/rkward/autotests/core_test.cpp
@@ -50,7 +50,7 @@ class RKWardCoreTest: public QObject {
 		bool *_done = &done;
 		connect(command->notifier(), &RCommandNotifier::commandFinished, this, [_done, callback](RCommand *command) { *_done = true; callback(command); });
 		RInterface::issueCommand(command, chain);
-		while (!done && t.elapsed() < timeoutms) {
+		while (!done && (t.elapsed() < timeoutms)) {
 			qApp->processEvents();
 		}
 		if (!done) {
@@ -98,7 +98,6 @@ private slots:
 		waitForAllFinished();
 	}
 	void initTestCase() {
-		qDebug("Initializing test case"); // Remove me. For diagnostics of test exception on Windows CI
 		qputenv("QTWEBENGINE_CHROMIUM_FLAGS", "--no-sandbox"); // Allow test to be run as root, which, for some reason is being done on the SuSE CI.
 		QLoggingCategory::setFilterRules("qt.text.layout=false");  // Filter out some noise
 		KAboutData::setApplicationData(KAboutData("rkward", "RKWard", RKWARD_VERSION, "Frontend to the R statistics language", KAboutLicense::GPL)); // component name needed for .rc files to load
@@ -213,6 +212,33 @@ private slots:
 		QCOMPARE(output.value(4), "444");
 	}
 
+	void cancelCommandStressTest() {
+		int cancelled_commands = 0;
+		int commands_out = 0;
+		for (int i = 0; i < 100; ++i) {
+			runCommandAsync(new RCommand("Sys.sleep(.005)", RCommand::User), nullptr, [&cancelled_commands, &commands_out](RCommand *command) {
+				if (command->wasCanceled()) cancelled_commands++;
+				commands_out++;
+			});
+			// We want to cover various cases, here, including cancelling commands before and after they have been sent to the backend, but also at least some commands that finish
+			// without being effictively cancelled.
+			if (i % 4 == 0) {
+				RInterface::instance()->cancelAll();
+			} else if (i % 4 == 1) {
+				while (commands_out < i) {
+					qApp->processEvents();
+				}
+			} else if (i % 4 == 2) {
+				qApp->processEvents();
+			}
+		}
+		waitForAllFinished();
+		// The point of this test case is to make sure, we do not get into a deadlock, however, the QVERIFYs below are to make sure the test itself behaves as expected.
+		// There needs to be some wiggle room, however, as this is inherently prone to race-conditions. (Commands finish running before getting cancelled, or they don't).
+		QVERIFY(cancelled_commands >= 25);
+		QVERIFY(cancelled_commands <= 75);
+	}
+
 	void priorityCommandTest() {
 		bool priority_command_done = false;
 		runCommandAsync(new RCommand("Sys.sleep(5)", RCommand::User), nullptr, [&priority_command_done](RCommand *command) {
diff --git a/rkward/rbackend/rkrinterface.cpp b/rkward/rbackend/rkrinterface.cpp
index 711026db..904d1312 100644
--- a/rkward/rbackend/rkrinterface.cpp
+++ b/rkward/rbackend/rkrinterface.cpp
@@ -199,9 +199,11 @@ void RInterface::tryNextCommand () {
 				command->status |= RCommand::Failed;
 
 				// notify ourselves...
-				RCommand* dummy = popPreviousCommand (command->id ());
-				RK_ASSERT (dummy == command);
-				handleCommandOut (command);
+				RCommand* dummy = popPreviousCommand(command->id ());
+				RK_ASSERT(dummy == command);
+				RK_DEBUG(RBACKEND, DL_DEBUG, "Not sending already cancelled command to backend");
+				handleCommandOut(command);
+				tryNextCommand();
 				return;
 			}
 
@@ -562,6 +564,7 @@ void RInterface::cancelCommand (RCommand *command) {
 	} else {
 		RK_ASSERT (false);
 	}
+	tryNextCommand();
 }
 
 void RInterface::pauseProcessing (bool pause) {