Fix supernova reboot bug, add reboot tests #3848

adcxyz · 2018-07-07T09:56:58Z

currently supernova servers tends to hang on reboot (macOS 10.13.4):

Server.supernova; 
s.reboot; // first time usually works
s.reboot; // second time usually hangs, because notify does not happen.

this is a timing issue: supernova sends back the quit message early, but the supernova process still lives long enough to interfere with the boot messaging traffic with the rebooted instance.
Thus, this PR adds waiting for release of the (previous) process PID when booting, which fixes the hang condition.
This PR includes

waiting for quitting process to release server.pid
safer testing for notify clientID info from server (supernova messages are not identical yet)
new unit tests for reboot method
fix to cycleNotify which tends to hang up TestServer_boot methods

Result: All tests invoving booted servers now run with supernova!
They all pass, except test_getn still fails because of a bug in supernova handle_s_getn #3841

I believe supernova will be part of the 3.10 release, yes?
If so, I would be happy if this bug fix went into 3.10, so supernova supported as fully as possible.

dont set clientID if no clientID from response

so we know when the process really has ended.

this is to avoid supernova hanging with incomplete boot state when rebooting.

supernova tends to gets stuck here for some reason.

telephon

Code reads well and TestServer_boot succeeds for both scsynth and supernova, tested on macOS.

telephon · 2018-07-07T10:46:09Z

SCClassLibrary/Common/Control/Server.sc

 			}, 0.25);
 		}
 	}

+	waitForPidRelease { |onDone, onFailure, timeout = 1|


usually we call this onComplete.

telephon · 2018-07-07T10:48:03Z

SCClassLibrary/Common/Control/Server.sc

+			} {
+				0.05.wait
+			};
+			if (pid.isNil) { onDone.value } { onFailure.value };


you could write if(pid.isNil, onDone, onFailure) , but well...

telephon · 2018-07-07T10:49:14Z

SCClassLibrary/Common/Control/Server.sc

@@ -922,6 +946,7 @@ Server {
 		} {
 			this.disconnectSharedMemory;
 			pid = unixCmd(program ++ options.asOptionsString(addr.port), { |exitCode|
+				pid = nil;
 				this.prOnServerProcessExit(exitCode);


shouldn't the line pid = nil happen in prOnServerProcessExit ?

either is fine by me - had it there first - lets see if @brianlheim has a preference

i also wonder whether theoretically a unixCmd could return a non nil pid even after it has called the exit function.

@telephon - do you mean, whether it's possible that unixCmd's callback could run while the process is still running? I looked over the associated code - I'm not an expert on the unixy stuff but it looks like it's a remote possibility? I think that would be an issue in the implementation of unixCmd though and IMO we're OK relying on it here.

I agree with @telephon that the line pid = nil should go in prOnServerProcessExit, makes this easier to understand

telephon · 2018-07-07T10:52:40Z

testsuite/classlibrary/TestServer_boot.sc

+		s.doWhenBooted {
+			// first boot OK, starting reboot from here
+			s.reboot {
+				s.doWhenBooted({


you could leave away the ({ here, too.

telephon

but please change at least the argument name to onComplete

adcxyz · 2018-07-07T12:10:40Z

ok all done,
and updated the reboot tests to use UnitTest.wait,
and fixed a float value for numWireBufs that stops supernova when booting.

mossheim

Thanks for tackling this! Comments below

mossheim · 2018-07-08T17:25:27Z

SCClassLibrary/Common/Control/Server.sc

 			}, 0.25);
 		}
 	}

+	waitForPidRelease { |onComplete, onFailure, timeout = 1|


this should be a private method (and hidden in documentation), right?

yes, I think so, too.

mossheim · 2018-07-08T17:25:47Z

SCClassLibrary/Common/Control/Server.sc

+			onComplete.value;
+			^this
+		};
+		// FIXME: quick and dirty fix for supernova reboot hang on osx:


This comment is good, thanks! Btw for future reference, the name is macOS now

mossheim · 2018-07-08T17:54:05Z

SCClassLibrary/Common/Control/Server.sc

@@ -922,6 +946,7 @@ Server {
 		} {
 			this.disconnectSharedMemory;
 			pid = unixCmd(program ++ options.asOptionsString(addr.port), { |exitCode|
+				pid = nil;
 				this.prOnServerProcessExit(exitCode);


@telephon - do you mean, whether it's possible that unixCmd's callback could run while the process is still running? I looked over the associated code - I'm not an expert on the unixy stuff but it looks like it's a remote possibility? I think that would be an issue in the implementation of unixCmd though and IMO we're OK relying on it here.

mossheim · 2018-07-08T17:55:20Z

SCClassLibrary/Common/Control/Server.sc

@@ -922,6 +946,7 @@ Server {
 		} {
 			this.disconnectSharedMemory;
 			pid = unixCmd(program ++ options.asOptionsString(addr.port), { |exitCode|
+				pid = nil;
 				this.prOnServerProcessExit(exitCode);


I agree with @telephon that the line pid = nil should go in prOnServerProcessExit, makes this easier to understand

mossheim · 2018-07-08T17:57:49Z

testsuite/classlibrary/TestServer_boot.sc

 		server.notify_(true);
-		while { server.notified.not } { 0.1.wait };
+		while { server.notified.not } { server.notify_(false); 0.1.wait };


for the sake of clarity, it would probably be better here to write statusWatcher.sendNotifyRequest(false). Then the code immediately makes sense with the comment.

mossheim · 2018-07-08T18:00:34Z

SCClassLibrary/Common/Control/Server.sc

+				}
+			} {
+				0.05.wait
+			};


please rewrite this using a Condition - waiting in a loop is inefficient and we have better idioms for this purpose

I'm not sure: do we have a callback somewhere that would make this possible? There would have to be a mechanism that unhangs the condition when the pid becomes nil. As long as we don't have that, this loop is the only possible way.

A possible way to implement it is that any code that changes the pid can signal a Condition that is waited on here. The Condition will have to be at class scope. pid == nil can even be used as the test function for the Condition.

If we do this, the condition has to be at object scope, otherwise booting servers will block each other (but this is probably what you meant, anyhow).

Unless there are other uses for a condition at that level, I'd prefer to keep the method self-contained. Polling is not bad practice as such at all, it just depends on the case. But I'm fine either way if it is elegant.

I don't think efficiency is an issue here - waiting until something has finished properly so you can continue is very cheap. As Julian points out, the polling solution here stays completely local; also, it is just a single local poll/wait thread, so there is no possible confusion of order of execution of waiting tasks.
IIRC, Condition.hangWithTimeout (which would be needed here) ran into problems that are still not resolved yet, right?
To me, polling seems the right choice here, and conditions would make it more complex than necessary.

This would a case for whenTrueWithin, as proposed in #3850.

Do whatever you want. This is not just about efficiency, but this code is going to be rewritten soon anyway.

yes, eg. when I get around to redoing the ServerProcess refactor :-)
which is hopefully soon

as requested in reviews

as requested in review

telephon · 2018-07-18T12:24:43Z

SCClassLibrary/Common/Control/Server.sc

@@ -280,6 +280,7 @@ Server {
 	var <window, <>scopeWindow, <emacsbuf;
 	var <volume, <recorder, <statusWatcher;
 	var <pid, serverInterface;
+	var <pidReleaseCondition;


is it necessary that this condition is externally accessible?

had it accessible for testing, happy to make it internal only

telephon · 2018-07-18T12:26:14Z

SCClassLibrary/Common/Control/Server.sc

@@ -339,6 +340,8 @@ Server {
 		this.name = argName;
 		all.add(this);

+		pidReleaseCondition = Condition({ this.pid == nil });


this sets the class pidReleaseCondition, but above it is defined as instance variable.

Like pid, pidReleaseCondition is an instance variable, because quitting is individual/independent per server instance. Not sure I understand your question?

but you are setting it in *initClass, here.

… I've just read the original code, false alarm, it is all in init. This is a github/diff artefact.

will go over this anyway in the ServerProcess refactor/cleanup, where every ServerProcess state gets its own condition, so this is just to make all tests with supernova to run now, rather than waiting for the ServerProcess refactor/cleanup to get through.

adcxyz · 2018-07-22T11:55:19Z

@telephon - removed condition getter,
so I think I addressed all your concerns, yes?
If so, then this is good to go.

adcxyz added 5 commits June 20, 2018 19:00

add tests for server.reboot with server off, and server on.

7d9b11e

improve handling of login response from supernova

a2332e9

dont set clientID if no clientID from response

let unixCmd end reset pid to nil on local and non-internal servers

4cf67db

so we know when the process really has ended.

before really booting, wait for qutting process to finish

177d363

this is to avoid supernova hanging with incomplete boot state when rebooting.

cycleNotify - resend notify messages until desired state

8710d88

supernova tends to gets stuck here for some reason.

adcxyz added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: supernova comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR labels Jul 7, 2018

adcxyz requested review from telephon and mossheim July 7, 2018 09:56

telephon approved these changes Jul 7, 2018

View reviewed changes

telephon requested changes Jul 7, 2018

View reviewed changes

adcxyz added 3 commits July 7, 2018 13:23

changed onDone to onComplete

b6c60aa

updated/simplified test methods to use UnitTest.wait

08b729e

move numWireBufs before server boot, add asInteger for supernova

48ddab8

telephon approved these changes Jul 7, 2018

View reviewed changes

mossheim reviewed Jul 8, 2018

View reviewed changes

adcxyz added 2 commits July 10, 2018 12:12

renamed waitForPidRelease, moved release to prOnServerProcessExit

84a2f52

as requested in reviews

clarified cycleNotify loops with explicit calls

ba3936d

as requested in review

miczac mentioned this pull request Jul 13, 2018

FAILURE IN SERVER /b_getn index out of range #3873

Closed

use Condition for prWaitForPidRelease.

ec59abd

nhthn changed the base branch from develop to 3.10 July 15, 2018 19:08

prWaitForPidRelease - replace timer Routine with sched

47825d0

telephon requested changes Jul 18, 2018

View reviewed changes

adcxyz added this to the 3.10 milestone Jul 22, 2018

Server: make instvar pidReleaseCondition internal.

c242628

telephon approved these changes Jul 22, 2018

View reviewed changes

telephon merged commit b53fc82 into supercollider:3.10 Jul 26, 2018

adcxyz deleted the fixRebootSupernova branch July 28, 2018 12:26

adcxyz mentioned this pull request Aug 12, 2018

supernova reboot not working properly #3794

Closed

mossheim added comp: testing comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR and removed comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR labels Mar 7, 2021

Fix supernova reboot bug, add reboot tests #3848

Fix supernova reboot bug, add reboot tests #3848

Conversation

adcxyz commented Jul 7, 2018

telephon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon Jul 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon left a comment

Choose a reason for hiding this comment

adcxyz commented Jul 7, 2018

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adcxyz commented Jul 22, 2018

telephon Jul 7, 2018 •

edited