Classlib: Lock client id while running #3275

adcxyz · 2017-11-03T00:54:40Z

This addresses and fixes #3272.

server has clientID settable while off, and locked while running.
-> maybe clientID can now be freely user-settable while off,
and we dont need userSpecifiedClientID anymore?
server.numClients gets set to maxLogins in scsynth response,
so the server keeps correct info about this
options.maxLogins is still settable as it was, to keep server.options independent
(as the options may be shared between servers)
method prHandleClientLoginInfoFromServer and prHandleNotifyFailString
moved to Server for clarity (they do change server state)

// tests for clientID locking:
s.quit;
s.notified; // false
s.clientIDLocked; // false
s.numClients; // 1
s.clientID;   // 0
s.boot;

s.clientIDLocked; // true
// can set options.maxLogins ...
s.options.maxLogins = 4;
// but numClients remembers 1 from scsynth
s.numClients;
// cannot set now
s.clientID_(3).clientID;
s.quit;

// now we can set maxLogins:
s.options.maxLogins = 6;
// and clientID
s.clientID_(3).clientID;
s.boot;
// but because clientID was not user-specified on server creation,
// it is reset to 0
s.clientID;

s.quit;
// when making a server with clientID specified, it is kept:
x = Server(\test, s.addr, s.options, 2);
x.boot;

// comes up identical after reboot
x.reboot;

// and tests run fine:
TestServer_clientID.run;
TestServer_clientID_booted.run;

* added numClients var for numClients/maxLogins as gotten from scsynth * clientID is locked while server is running and notified. * moved prHandle ... methods to Server

telephon

Looks good, just some minor things.

telephon · 2017-11-03T08:17:48Z

SCClassLibrary/Common/Control/Server.sc

@@ -267,6 +267,7 @@ Server {

 	var <name, <addr, <clientID, <userSpecifiedClientID = false;
 	var <isLocal, <inProcess, <>sendQuit, <>remoteControlled;
+	var numClients; // maxLogins as sent from booted scsynth


why not call it maxLogins? Hm, I understand that clientID and numClients are a pair.

I find that maxLogins is a only technical name, numClients is more semantic:
it is the number of clients that the server is prepared to handle.

well, you could read it as the number of clients the server currently has.

telephon · 2017-11-03T08:18:48Z

SCClassLibrary/Common/Control/Server.sc

@@ -351,13 +353,13 @@ Server {

 	}

+	numClients { ^numClients ?? { options.maxLogins } }


in such simple cases it is more efficient to write numClients ? options.maxLogins

telephon · 2017-11-03T08:21:00Z

SCClassLibrary/Common/Control/Server.sc

 		this.sendDefaultGroups;
 		tree.value(this);
 		ServerTree.run(this);
 	}

 	/* id allocators */

-	// private, called from server notify response with next free clientID
+	clientIDLocked { ^this.serverRunning and: this.notified }


IIRC, when you want to inline the test, you need to write this.serverRunning and: { this.notified }

telephon · 2017-11-03T08:40:00Z

SCClassLibrary/Common/Control/Server.sc

@@ -410,6 +421,65 @@ Server {
 		};
 	}

+	prHandleClientLoginInfoFromServer { |newClientID, newMaxLogins|


Could you perhaps move the private implementations further down? It is better to have the public interface coming first. E.g. here: https://github.com/adcxyz/supercollider/blob/8be07637fb9255c52c3a908a14d0b46b6f140e80/SCClassLibrary/Common/Control/Server.sc#L582

jamshark70 · 2017-11-03T13:21:51Z

SCClassLibrary/Common/Control/Server.sc

@@ -351,13 +353,13 @@ Server {

 	}

+	numClients { ^numClients ? options.maxLogins }


FWIW: In my testing, a ? b is no more and no less than a ?? { b } (sometimes one is faster, sometimes the other is, randomly). But if a is not nil, a ? b.method is always 20-25% slower than a ?? { b.method }. Let's not overextend that one surprising finding from awhile ago. In the present case here, ?? is better.

ok! I once tested this and found the ?? to be slower in general for single instvar lookups. But, whatever, this case counts.

rename clientIDLocked to isFullyBooted for clarity.

(doWhenBooted is tested implicitly in waitForBoot)

adcxyz · 2017-11-08T18:35:33Z

added unit tests for waitForBoot and bootSync, and they pass.

telephon · 2017-11-10T08:03:23Z

SCClassLibrary/Common/Control/Server.sc

 	}

 	/* id allocators */

-	// private, called from server notify response with next free clientID
+	isFullyBooted { ^this.serverRunning and: { this.notified } }


Actually, thinking about the naming: To me it seems that server.serverRunning should return only ever true when it is notified.

so I'd suggest:

var <>hasBooted; serverRunning { ^this.hasBooted and: { this.notified } }

This might mean more changes internally. But it is correct semantically. Otherwise we'll run into allocator troubles.

E.g. when you start a NodeProxy it should warn also when the server hasn't been notified yet.

good point - yes can try that. a bit hairy to get to work properly, but should leave booting cleaner for the next reworking.

will you still add it in this PR?

telephon · 2017-11-10T08:09:56Z

testsuite/classlibrary/TestServer_boot.sc

+		s.remove;
+		of.free;
+	}
+


It would be good to add a test like that:

( var done = false, prevNodeID, nodeID = -1; fork { s.boot; 1000.do { if(s.serverRunning) { nodeID = s.nextNodeID; }; if(prevNodeID == nodeID) { "error".warn; }; prevNodeID = nodeID; 0.001.wait; } } )

good idea, will do

jamshark70 · 2017-11-11T06:04:50Z

Just checking, what are the pending reviews on this? IMO the fix for waitForBoot should be mandatory for beta2.

I see:

Maybe change the name of numClients
numClients { ^numClients ? options.maxLogins } change back to use ?? { ... }
Rethink isFullyBooted logic (though, there's a good case to be made for not fiddling with it too much now, and dealing with it in the more comprehensive rework)
One nodeID test case.

adcxyz · 2017-11-11T08:16:05Z

@jamshark70
Just checking, what are the pending reviews on this? IMO the fix for waitForBoot should be mandatory for beta2.

I see:

Maybe change the name of numClients

yes - I would prefer keeping 'clients' in the name. for clarity, could be maxNumClients,
so numClients is free for the number of currently logged-in clients (once there is a way to request that info)

numClients { ^numClients ? options.maxLogins } change back to use ?? { ... }

yes, done.

Rethink isFullyBooted logic (though, there's a good case to be made for not fiddling with it too much now, and dealing with it in the more comprehensive rework)

yes.

One nodeID test case.

yes.

I have two more items on the optional list:

test cases for four ways of making sound with s.waitForBoot
( { }.play; ().play; Synth(\default); Ndef(\x, { VarSaw.ar }).play; )
remove userSpecifiedClientID to simplify server config.
now there is a clear logic when clientIDs can be changed,
there is no need for that distinction anymore.

adcxyz · 2017-11-11T09:08:19Z

and one more:

remove Server:remoteControlled -it seems unused, and isLocal is sufficient for potential uses of remoteControlled.

telephon · 2017-11-11T10:02:57Z

the idea of remoteControlled was that you could also take control over the boot process of a local server from some other application. If we really need that I am not sure, but that was the intention.

(remoteControlled has a setter)

telephon · 2017-11-11T10:05:23Z

example

z = Server(\test, NetAddr("127.0.0.1", 57878));
z.remoteControlled = true;

z.boot;

adcxyz · 2017-11-11T14:46:33Z

the idea of remoteControlled was that you could also take control over the boot process of a local server from some other application. If we really need that I am not sure, but that was the intention.

(remoteControlled has a setter)

sorry if I am slow here, but this is still not fully clear to me:
do you mean booting scsynth from sclang FOR another application (on the same computer or oever network) that will use to it to play sound?
in that case, it just has to be local, so sclang can boot it, and the other app needs its addr.

what is the extra information contained in remoteControlled?

Otherwise, IF the other app wanted to boot scsynth, it would have to be local,
and know enough about scsynth to shell-start it with a proper config string, no?
and then why should sclang have a server instance for that process?

just asking to understand it - fine to leave it alone for now!

telephon · 2017-11-11T17:17:17Z

Let's assume sclang and scsynth run on the same machine, but sclang has no privileges to boot scsynth. Then scsynth needs to be booted by hand.

If you don't set remoteControlled = true, then sclang will try to boot the scsynth application (because it is local, given its address), but fail.

That was IIRC, the scenario.

mossheim · 2017-11-12T20:09:48Z

@adcxyz - any chance you could rebase this on 3.9 head? I didn't realize it but this now conflicts because you moved some methods from ServerStatus to Server

mossheim · 2017-12-22T00:16:06Z

After adcxyz#9 is merged, I can approve this :) then I think it will be ready for merge.

jamshark70 · 2017-12-22T00:33:14Z

I've been running this for a couple of weeks now -- seems to be working. I agree -- I think it's ready to merge.

mossheim · 2017-12-22T00:36:49Z

Great! Thank you so much for trying it out @jamshark70

Cleanup

nhthn · 2017-12-23T22:01:30Z

will make some attempt to look at this today and tomorrow. thanks all for working on this!

nhthn · 2017-12-24T17:56:20Z

the title and top post of this PR seem to be out of date. can someone give me a complete and up to date list of the intended changes in this PR?

mossheim · 2017-12-24T19:25:38Z

@snappizz

Fixes protect clientID_ setter in server? #3272
Fixes Server#waitForBoot is broken #3280
Fixes internal server does not fully boot and has 64 maxLogins #3283
Server's clientID property can only be modified while not running
Server.userSpecifiedClientID is deprecated
Server's maxNumClients property is set to maxLogins from server's response
options.maxLogins is still settable as it was, to keep server.options independent (as the options may be shared between servers)
prHandleClientLoginInfoFromServer and prHandleNotifyFailString moved to Server for clarity (they do change server state)
Many tests added to verify server boot sequence, including GUI updating and client ID modification and Volume synth creation
Documentation added and updated for Server and ServerStatusWatcher
Server.allBootedServers added to give access to a list of booted (but not necessarily running) servers
Server.hasBooted added. Signals that server is not yet "running" (meaning that it hasn't received a response to a notify request yet) but has fully booted.
SynthDesc and SynthDef now only send to booted servers
Update messages passed to Server GUI are properly deferred (not sure if this was a real issue before)
Volume's behavior was fixed to make it compatible with changes in this PR. It respects its assigned server, and sends its synthdef if the server hasBooted (as opposed to serverRunning)

See the documentation for hasBooted and serverRunning for an explanation of their guarantees. The rest of the code will make much more sense if you've read that first.

Other than that, I think this is actually a case where the tests document the design, so look those over if you still have questions.

nhthn · 2017-12-24T22:52:19Z

geez that is a lot. thanks for the info.

nhthn · 2017-12-25T07:24:47Z

SCClassLibrary/Common/Control/Server.sc

@@ -725,13 +773,9 @@ Server {
 		^Buffer.cachedBufferAt(this, bufnum)
 	}

-	// defaultGroups for all clients on this server:
-
-	allClientIDs { ^(0..options.maxLogins-1) }


should this be properly deprecated rather than hard removal?

this hasn't been part of any release, it was added in a recent PR

nhthn · 2017-12-25T07:27:18Z

testsuite/classlibrary/TestServer_clientID_booted.sc

+		ServerTree.add(func, s);
+		this.bootServer(s);
+
+		synth2 = Synth("default", [\freq, 330]);


let's try to avoid adding new noises to the test suite.

IMO it's up to you to change it then; there's been no agreement about that and everyone else is fine with it

telephon

the one request isn't so important, so I approve as it is now.

telephon · 2017-12-25T16:17:59Z

SCClassLibrary/Common/Audio/SynthDesc.sc

-		global.send(server, tryToLoadReconstructedDefs);
+		if (server.hasBooted) {
+			global.send(server, tryToLoadReconstructedDefs);
+		}


maybe warn if server hasn't booted.

nhthn · 2017-12-25T18:29:44Z

tests are failing for me:

a TestServer_ServerGUI:test_noBoot - Before booting: 'boot' button should have correct value 
Is:
     1 
Should be:
     0 

a TestServer_ServerGUI:test_noBoot - Before booting: 'record' button should be disabled
a TestServer_GUI:test_serverBoot - ServerBoot should be able to update GUI objects in the main function

mossheim · 2017-12-25T18:54:16Z

@snappizz I can't reproduce. Look into a little bit and see if you can find why the test fails.

nhthn · 2017-12-25T19:46:02Z

the failing tests happen when i run all unit tests rather than just selectively running tests. i have figured out how to reproduce them in isolation as well: boot interpreter, boot server, then run either TestServer_ServerGUI.run or TestServer_GUI.run (but not both).

(also feel free to hop on slack if we want to avoid expanding this nearly record-length comment section even further)

in TestServer_GUI

in TestServer_ServerGUI

in TestVolume

Avoid using default server in test suite

nhthn · 2017-12-26T23:28:30Z

current test failures:

a TestNodeProxy:test_load_init - after server quit: no resources should be on the server
a TestPatternProxy:test_update_withEvents - testing '1.1' failed.
a TestPatternProxy:test_update_withEvents - testing '1.2' failed.
a TestPatternProxy:test_update_withEvents - testing '2.1' failed.
a TestPatternProxy:test_update_withEvents - testing '2.2' failed.
a TestPatternProxy:test_update_withEvents - testing '2.3' failed.
a TestPatternProxy:test_update_withEvents - testing '3.1' failed.
a TestPatternProxy:test_update_withEvents - testing '3.2' failed.
a TestPatternProxy:test_update_withEvents - testing '3.3' failed.
a TestPatternProxy:test_update_withEvents - testing '4.1' failed.
a TestPatternProxy:test_update_withEvents - testing '4.2' failed.
a TestPatternProxy:test_update_withEvents - testing '5.1' failed.
a TestPatternProxy:test_update_withEvents - testing '5.2' failed.
a TestPatternProxy:test_update_withEvents - testing '5.3' failed.
a TestPatternProxy:test_update_withEvents - testing '5.4' failed.
a TestPatternProxy:test_update_withEvents - testing '7.1' failed.
a TestPatternProxy:test_update_withEvents - testing '7.2' failed.
a TestPatternProxy:test_update_withEvents - testing '7.3' failed.
a TestCoreUGens:test_ugen_generator_equivalences - "Duty.ar(SampleDur.ir, 0, x) == x"
1 of 48000 items in array failed to match. Displaying arrays from index of first failure (0) onwards:
FloatArray[ 0.40459442138672, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,...etc...
! = 
0

a TestCoreUGens:test_ugen_generator_equivalences - "Duty.ar(SampleDur.ir, 0, Drand([x],inf)) == x"
1 of 48000 items in array failed to match. Displaying arrays from index of first failure (0) onwards:
FloatArray[ 1.1653399467468, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...etc...
! = 
0

a TestCoreUGens:test_demand - Duty should free itself after a limited sequence

i think the TestCoreUGens failure are unrelated to this PR. however i can't seem to successfully run tests on our current 3.9 branch for comparison, it simply craps out when it tries TestDuty.

ServerQuit should not run when server boots

if this.serverRunning is set to false before other state variables, ServerQuit doesn't get called cleaned up some logic in serverRunning_ so that ServerQuit runs when it should (i.e. not twice at boot)

mossheim · 2017-12-27T01:48:21Z

Some of the failures @snappizz points out above are fixed in #3340. Some are due to test code in TestCoreUGens that ought to be refactored. The first one, however, caught a pretty significant issue in this PR that hadn't been covered by a unit test yet - ServerQuit was running (twice) on boot, but not on quit. I have just pushed a test and corresponding fix for this, and confirmed on my machine that the entire test suite passes except for these known failures.

nhthn

thanks to all, i think this is good to go. there are still some issues with this PR (most significantly, i would much prefer a read-only ServerStatusWatcher:serverRunning rather than a setter with side effects) but they can safely be put off until later.

adcxyz added 3 commits November 3, 2017 01:29

Server: lock clientID and numClients while running.

f819563

* added numClients var for numClients/maxLogins as gotten from scsynth * clientID is locked while server is running and notified. * moved prHandle ... methods to Server

fix assignment of numClients in response handler.

3597500

TestServerClientID - cleanup after test.

8be0763

telephon requested changes Nov 3, 2017

View reviewed changes

adcxyz added 2 commits November 3, 2017 11:20

minor changes as requested by @telephon

6b618af

TestServer_clientIDBooted - cleanup after test.

72917f8

telephon approved these changes Nov 3, 2017

View reviewed changes

jamshark70 reviewed Nov 3, 2017

View reviewed changes

mossheim added the comp: class library SC class library label Nov 4, 2017

adcxyz added 5 commits November 6, 2017 22:24

server: doWhenBooted waits until server is fully booted.

14eeeeb

rename clientIDLocked to isFullyBooted for clarity.

Server:initTree - wrap ServerTree.run in a Routine so it can sync.

54ede2f

Volume: only sendSynthDef to my server, not to all.

0dca048

added unit tests for waitForBoot and bootSync

aa0885c

(doWhenBooted is tested implicitly in waitForBoot)

added test for clientID locked while server is running.

c420625

adcxyz mentioned this pull request Nov 8, 2017

Server#waitForBoot is broken #3280

Closed

Server.internal: make sure .boot starts watcher, don't reset maxLogins.

4b40d48

adcxyz mentioned this pull request Nov 9, 2017

internal server does not fully boot and has 64 maxLogins #3283

Closed

telephon requested changes Nov 10, 2017

View reviewed changes

adcxyz added 3 commits November 11, 2017 09:18

Server: use ?? ? where it is cleaner.

2934e82

Server: change numClients to maxNumClients

cd43bd5

Server: remove userSpecifiedClientID

b4559af

help: add help file for ServerStatusWatcher

a7fb558

mossheim mentioned this pull request Dec 22, 2017

Cleanup adcxyz/supercollider#9

Merged

Merge pull request #9 from brianlheim/lcidwr/cleanup

640026f

Cleanup

mossheim approved these changes Dec 22, 2017

View reviewed changes

nhthn self-requested a review December 23, 2017 22:01

nhthn self-assigned this Dec 24, 2017

nhthn reviewed Dec 25, 2017

View reviewed changes

telephon approved these changes Dec 25, 2017

View reviewed changes

nhthn mentioned this pull request Dec 25, 2017

[3.8] Update changelog and version number for 3.8.1 #3343

Merged

mossheim and others added 4 commits December 25, 2017 16:32

testsuite: avoid using default server

e31b319

in TestServer_GUI

testsuite: avoid using default server

ada282e

in TestServer_ServerGUI

testsuite: avoid using default server

3abcd3e

in TestVolume

Merge pull request #10 from brianlheim/lcidwr/tests-server

f540347

Avoid using default server in test suite

mossheim added 2 commits December 26, 2017 19:41

test suite: add test for ServerQuit

6ea980c

ServerQuit should not run when server boots

classlib: server: update boot sequence

73e5242

if this.serverRunning is set to false before other state variables, ServerQuit doesn't get called cleaned up some logic in serverRunning_ so that ServerQuit runs when it should (i.e. not twice at boot)

nhthn approved these changes Dec 27, 2017

View reviewed changes

mossheim merged commit 2c7f95f into supercollider:3.9 Dec 27, 2017

adcxyz deleted the lockClientIDWhileRunning branch April 4, 2018 08:29

		@@ -351,13 +353,13 @@ Server {

		}

		numClients { ^numClients ?? { options.maxLogins } }

		@@ -351,13 +353,13 @@ Server {

		}

		numClients { ^numClients ? options.maxLogins }

Classlib: Lock client id while running #3275

Classlib: Lock client id while running #3275

Conversation

adcxyz commented Nov 3, 2017

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

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 Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

adcxyz commented Nov 8, 2017

telephon Nov 10, 2017 • edited Loading

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

jamshark70 commented Nov 11, 2017

adcxyz commented Nov 11, 2017

adcxyz commented Nov 11, 2017

telephon commented Nov 11, 2017 • edited Loading

telephon commented Nov 11, 2017

adcxyz commented Nov 11, 2017

telephon commented Nov 11, 2017

mossheim commented Nov 12, 2017

mossheim commented Dec 22, 2017 • edited Loading

jamshark70 commented Dec 22, 2017

mossheim commented Dec 22, 2017

nhthn commented Dec 23, 2017

nhthn commented Dec 24, 2017

mossheim commented Dec 24, 2017 • edited by telephon Loading

nhthn commented Dec 24, 2017

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

Choose a reason for hiding this comment

nhthn commented Dec 25, 2017

mossheim commented Dec 25, 2017 • edited Loading

nhthn commented Dec 25, 2017

nhthn commented Dec 26, 2017 • edited Loading

mossheim commented Dec 27, 2017

nhthn left a comment

Choose a reason for hiding this comment

telephon Nov 3, 2017 •

edited

Loading

telephon Nov 10, 2017 •

edited

Loading

telephon commented Nov 11, 2017 •

edited

Loading

mossheim commented Dec 22, 2017 •

edited

Loading

mossheim commented Dec 24, 2017 •

edited by telephon

Loading

mossheim commented Dec 25, 2017 •

edited

Loading

nhthn commented Dec 26, 2017 •

edited

Loading