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

Proposal: Split duties between Server and ServerProcess #3310

Open
adcxyz opened this issue Nov 24, 2017 · 12 comments
Open

Proposal: Split duties between Server and ServerProcess #3310

adcxyz opened this issue Nov 24, 2017 · 12 comments
Assignees
Labels
comp: class library SC class library

Comments

@adcxyz
Copy link
Contributor

adcxyz commented Nov 24, 2017

In the discussion on #3286 and complicated PR #3299,
@jamshark70 had the insight that ServerStatusWatcher should actually be responsible for handling the entire admin for the server process: booting, watching status, and quitting.

I have started to explore this idea in a branch:
https://github.com/adcxyz/supercollider/tree/server_ServerProcess

This is a massive refactoring of Server, ServerStatusWatcher based on #3299,
and looks promising so far - not at all finished yet, so not a PR yet.

Now the ServerProcess class replaces ServerStatusWatcher
and handles boot, quit, and running status of a server process.
The status code is copied and adapted from ServerStatusWatcher,
boot and quit-related methods are copied and refactored from Server.

The Server class now focuses only on 'content': allocation, synths, controls, etc.

*** Disclaimers :

  • This shifts a lot of code around in ways that github cannot track well
  • A lot of things are renamed for clarity. I made an effort to support
    much of the earlier code interface, but this is not complete at all.

*** Pluses:

  • all boot steps are in a single boot routine

  • logic is a lot clearer and easier to read

  • uses conditions to speed up booting and trigger waiting tasks

  • process state is always one of #[\isOff, \isBooting, \isSettingUp, \isReady, \isQuitting];

  • other state flags are kept for internal use, such as hasBooted, unresponsive

  • ca. 40% faster booting on macOS (measured ca. 0.65 sec vs 1.15 sec avg)
    Opinions welcome!

  • For all current doc and tests, see ServerProcess.help

*** TODO: ***

  • UnitTests for every if-branch in boot method!
//////// CODE examples - WORKING ALREADY : ///////////
// These tests all run:
TestServer_boot.run;
TestServer_clientID.run;
TestServer_clientID_booted.run;

Server.trace; // enables extensive posts on all boot steps for debugging 

// * boot has onComplete and onFailure arguments:
s.boot(onComplete: { "READY".postln; }, onFailure: { "failed".postln; }, timeout: 5);

// * all standard boot variants already work
s.reboot;
s.quit;
s.doWhenBooted { "AAA".postln };
s.doWhenBooted { "BBB".postln };
s.waitForBoot { Env.perc.test };

// For all current doc and tests, see 
ServerProcess.help;
@mossheim mossheim added the comp: class library SC class library label Nov 25, 2017
@mossheim mossheim added this to the future milestone Nov 25, 2017
@adcxyz
Copy link
Contributor Author

adcxyz commented Nov 25, 2017

Yay, got user accessible conditions and doWhenBooted working nicely now:

(
s.quit;

"XYZ".do { |char, i|
	Routine {
		"% is waiting for booted ...\n".postf(char);
		s.process.hasBootedCondition.wait;
		"% is waiting for server to be ready ...\n".postf(char);
		s.process.isReadyCondition.wait;
		3.do { ["   ", i, char].join($ ).postln; 0.1.wait };
		"% is waiting for server to quit ...\n".postf(char);
		s.process.hasQuitCondition.wait;
		"% ends.\n".postf(char);
	}.play;
};
)
s.boot;

-> X is waiting for server to be ready ...
Y is waiting for server to be ready ...
Z is waiting for server to be ready ...
*** server localhost is ready after 0.2535 secs!
    0 X
    1 Y
    2 Z
    0 X
    1 Y
    2 Z
    0 X
    1 Y
    2 Z
X is waiting for server to quit ...
Y is waiting for server to quit ...
Z is waiting for server to quit ...

s.quit; // trigger ends 

-> localhost
X ends.
Y ends.
Z ends.


(
s.quit;

s.doWhenBooted({ "AAA".postln; });
s.doWhenBooted({ "BBB".postln; });
s.doWhenBooted({ "CCC".postln; });

// raw waiting routines run first
"XYZ".do { |char, i|
	Routine {
		"% is waiting for server to be ready ...\n".postf(char);
		s.process.isReadyCondition.wait;
		"% runs:\n".postf(char);
		
		3.do { ["   ", i, char].join($ ).postln; 0.1.wait };
	}.play;
};

s.doWhenBooted({ "DDD".postln; });
s.doWhenBooted({ "EEE".postln; });
s.doWhenBooted({ "FFF".postln; });
)

s.waitForBoot({ "waitForBoot runs...".postln });

X runs:
    0 X
Y runs:
    1 Y
Z runs:
    2 Z
waitForBoot runs...
AAA
BBB
CCC
DDD
EEE
FFF
    0 X
    1 Y
    2 Z
    0 X
    1 Y
    2 Z

@adcxyz
Copy link
Contributor Author

adcxyz commented Nov 25, 2017

dear @brianlheim, thanks for pointing out conditions,
dear @jamshark70, thanks for the clean-split of duties insight,
dear @telephon thanks for the ServerStatusWatcher approach;
curious what you think of this implementation now :-)
there is even a help file now...

@jamshark70
Copy link
Contributor

I'm afraid I'm in the middle of an apartment move, not much time for careful review. I'm glad to see that you ran with it -- eager to take a closer look, but overwhelmed at the moment.

@adcxyz
Copy link
Contributor Author

adcxyz commented Nov 26, 2017

@jamshark70 - no hurry, and still too early for a detailed review!

  • what would help me is people doing a quick browse through the ServerProcess help file examples and tests, and tell me if they see any general flaw with the approach :-)

@telephon
Copy link
Member

Looks like a very good direction to move towards!

Here a first error report:

SC_AudioDriver: sample rate = 44100.000000, driver's block size = 512
SuperCollider 3 server ready.
Requested notification messages from server 'localhost'
Requested notification messages from server 'localhost'
/notify : already registered
localhost: no maxLogins info from server process.
localhost: keeping clientID (0) as confirmed by server process.
localhost: no maxLogins info from server process.
localhost: keeping clientID (0) as confirmed by server process.
FAILED AT END OF BOOTSTAGE2
****** 'localhost' bootAt 0.4211 : 
****** BOOT FAILED OR TIMED OUT!
****** running onFailure.

when running

s.quit;
Routine {
    var name = "Routine A";
    "% waiting for booting to start ...\n".postf(name);
    s.process.startedBootingCondition.wait;
    "% boot started, waiting for hasBooted ...\n".postf(name);
    s.process.hasBootedCondition.wait;
    "% booted, waiting for server ready ...\n".postf(name);
    s.process.isReadyCondition.wait;
    "% READY! now waiting for server quit ...\n".postf(name);
    s.process.hasQuitCondition.wait;
    "% ends, quit done.\n".postf(name);
}.play;

s.boot;

@adcxyz
Copy link
Contributor Author

adcxyz commented Nov 28, 2017

@telephon - thanks for testing! I guess you ran did you run
s.quit; ... Routine { ... }.play; ... s.boot;
in one single go, right?
s.quit.bootwas broken, works now!

@adcxyz adcxyz modified the milestones: future, 3.9.1 Nov 28, 2017
@adcxyz adcxyz self-assigned this Nov 28, 2017
@jamshark70
Copy link
Contributor

Quick initial report: wslib will need an update for this.

ERROR: Variable 'pid' not defined.
  in file '/home/dlm/.local/share/SuperCollider/downloaded-quarks/wslib/wslib-classes/Extensions/Server/extServer-bootInTerminal.sc'
  line 7 char 25:

  			pid = thisProcess.pid;
                           
  		},{

Well, extension authors should be careful about referencing object internals anyway (program to the interface, not the implementation...).

@jamshark70
Copy link
Contributor

I'm testing this branch now. One thing (not sure if it's already reported) -- if the server process dies, nobody notices.

s.boot;
...
*** server localhost is ready after 0.9372 secs!

// do something externally to kill the server process
// e.g. in Linux, stop the JACK server
->
JackDriver: killed by jack
RESULT = 0

// then
s.serverRunning
-> true

@jamshark70
Copy link
Contributor

I just got this:

Requested notification messages from server 'localhost'
localhost: server process's maxLogins (1) matches with my options.
localhost: keeping clientID (0) as confirmed by server process.
FAILED AT END OF BOOTSTAGE2
****** 'localhost' bootAt 1.2718 : 
****** BOOT FAILED OR TIMED OUT!
****** running onFailure.

But

$ ps x | grep scsynth
 9298 ?        SLl    0:00 scsynth -u 57110 -a 1024 -i 2 -o 2 -b 1026 -m 131072 -R 0 -C 0 -l 1

So there is some weird possible race condition where the server boots successfully, but ServerProcess thinks it failed.

The failure was basically immediate.

I had recompiled the class library, then ran a waitForBoot code block.

I have to prepare some SC things for class, no time for this now, I have to go back to vanilla 3.9 for the moment. Otherwise I would investigate, but... this is weird.

@mossheim mossheim modified the milestones: 3.9.1, 3.10 Dec 2, 2017
@mossheim
Copy link
Contributor

mossheim commented Dec 2, 2017

Milestoning this as 3.10; it's not really appropriate for a patch release.

@telephon
Copy link
Member

@adcxyz where are we with all this? Is there a small first step to be made for 3.10, or better a larger one for 3.11 ?

@adcxyz
Copy link
Contributor Author

adcxyz commented Jun 19, 2018

Since so many details in Server.sc have changed, it needs a rewrite based on the current develop branch; the old version worked pretty well, and can be used as a model for the structure.

Useful small steps would be moving ServerOptions and ServerShmInterface to separate files...
#3794 is yet another pesky inconsistency that could be cleaned up nicely along the way, but I think it is more urgent than 3.11, so will likely get a less-than-elegant temp fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

No branches or pull requests

5 participants