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.

Sign up for GitHub

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

fix re-login behavior of remote clients to a server #3486

Merged
merged 11 commits into from
Mar 14, 2018
Merged

fix re-login behavior of remote clients to a server #3486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
074be48
fix re-login of remote client to a server: gets old clientID and corr…
Feb 1, 2018
a61421c
test_recoverRemoteReLogin: add assert for serverRunning status.
Feb 1, 2018
8847e40
Merge branch '3.9' into fixRecoverRemoteLogin
Feb 5, 2018
c9937b0
improve clarity by renaming, post info for handled cases.
Feb 5, 2018
511fca1
improve tests by renaming, using conditions, and more cases.
Feb 5, 2018
5b482fb
remove unneeded wait from test
Feb 6, 2018
0832967
make emergency advice post well noticeable
Feb 6, 2018
5472a7c
split remoteLogin in 2 tests, preapre for kill method.
Feb 6, 2018
8f42e17
add temp-replacement for killProcessByID so crossplatform tests pass
Feb 9, 2018
02f0075
use new Platform:killProcessByID method instead of local temp
Mar 1, 2018
eb836cb
test_repeatedLogin: remove commented-out cruft.
Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion SCClassLibrary/Common/Control/Server.sc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,11 @@ Server {
// post info on some known error cases
case
{ failString.asString.contains("already registered") } {
// when already registered, msg[3] is the clientID by which
// the requesting client was registered previously
"% - already registered with clientID %.\n".postf(this, msg[3]);
statusWatcher.notified = true;
statusWatcher.prHandleLoginWhenAlreadyRegistered(msg[3]);

} { failString.asString.contains("not registered") } {
// unregister when already not registered:
"% - not registered.\n".postf(this);
Expand Down
52 changes: 40 additions & 12 deletions SCClassLibrary/Common/Control/ServerStatus.sc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,45 @@ ServerStatusWatcher {

// final actions needed to finish booting
prFinalizeBoot {
ServerBoot.run(server);
server.sync;
server.initTree;
// this needs to be forked so that ServerBoot and ServerTree will definitely run before
// notified is true.
fork({
ServerBoot.run(server);
server.sync;
server.initTree;

this.notified = true;
server.changed(\serverRunning);
}, AppClock)
}

// This method attempts to recover from a loss of client-server contact,
// which is a serious emergency in live shows. So it posts a lot of info
// on the recovered state, and possibly helpful next user actions.

prHandleLoginWhenAlreadyRegistered { |clientIDFromProcess|
"% - handling login request though already registered - \n".postf(server);

// by default, only reset clientID if changed, to leave allocators untouched:
if (clientIDFromProcess != server.clientID) {
// make sure we can set the clientID, and set it
notified = false;
server.clientID_(clientIDFromProcess);
"*** This seems to be a login after a crash, or from a new server object,\n"
"*** so you may want to release currently running synths by hand:".postln;
"%.defaultGroup.release;\n".postf(server.cs);
"*** and you may want to redo server boot finalization by hand:".postln;
"%.statusWatcher.prFinalizeBoot;\n\n".postf(server.cs);
} {
// same clientID, so leave all server resources in the state they were in!
"This seems to be a login after a loss of network contact - \n"
"- reconnected with the same clientID as before, so probably all is well.\n".postln;
};

// ensure that statuswatcher is in the correct state immediately.
this.notified = true;
unresponsive = false;
server.changed(\serverRunning);
}

prSendNotifyRequest { |flag = true, addingStatusWatcher|
Expand All @@ -252,15 +288,7 @@ ServerStatusWatcher {
// XXX: this is a workaround because using `serverBooting` is not reliable
// when server is rebooted quickly.
if(addingStatusWatcher) {
// this needs to be forked so that ServerBoot and ServerTree will definitely run before
// notified is true.
fork({
this.prFinalizeBoot;

// serverRunning will now return true, and serverBooting will be marked as false
notified = true;
server.changed(\serverRunning);
}, AppClock);
this.prFinalizeBoot;
} {
notified = true;
}
Expand Down
146 changes: 146 additions & 0 deletions testsuite/classlibrary/TestServer_clientID_booted.sc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,150 @@ TestServer_clientID_booted : UnitTest {
s.quit;
s.remove;
}

// test that repeated login from the same server object to a running server process
// ends with correct server status, and keeps clientID and allocators as is.
test_repeatedLogin {
var server, cond, timer, lastNodeID;

server = Server(thisMethod.name.asSymbol);
server.options.maxLogins = 4;
server.clientID = 3;
this.bootServer(server);

// use some nodeIDs so we know whether nodeID allocator is reset
10.do { server.nextNodeID };
lastNodeID = server.nextNodeID;

// simulate losing network contact by turning off watcher,
// but actually leave the server process running
server.statusWatcher
.stopStatusWatcher
.stopAliveThread
.notified_(false)
.serverRunning_(false);

// now login again from the same server object
// should get "This seems to be a login after loss of network ... all is well" post.
server.startAliveThread;

cond = Condition.new;

timer = fork { 3.wait; cond.unhang };
server.doWhenBooted {
"% - server login timed out.\n".postf(thisMethod);
timer.stop;
cond.unhang;
};

cond.hang;

this.assert(server.serverRunning,
"after repeated login, server should be known to be fully running."
);

this.assert(server.clientID == 3,
"after repeated login, remote client should get the same clientID from server process."
);

this.assert(server.nextNodeID > lastNodeID,
"after repeated login, nodeID allocator should not be reset."
);

server.quit;
server.remove;

}

// test that logins to a (pseudo-) remote server process work as intended
// first login initializes fully,
// second login from same address gets the same clientID, and sets running state.
test_remoteLogin {
var options, serverPid, remote1, cond, timer;

options = ServerOptions.new;
options.maxLogins = 4;

// simulate a remote server process by starting a server process independently of SC
serverPid = unixCmd(Server.program + options.asOptionsString);

// login with standard Server.remote method to test for
remote1 = Server.remote(\remoteLogin1, options: options, clientID: 3);
cond = Condition.new;
timer = fork { 3.wait; cond.unhang };

remote1.doWhenBooted {
"% - server first login timed out.\n".postf(thisMethod);
timer.stop;
cond.unhang;
};
cond.hang;

this.assert(remote1.serverRunning,
"after first login, remote client should be known to be fully running."
);

this.assert(remote1.clientID == 3,
"after first login, remote client should have its requested clientID."
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these asserts necessary? they seem to be covered well by other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the Server:remote method is not tested anywhere else, so testing both its normal and its emergency use seemed efficient and appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it should be in a separate test...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so have a test for remoteLogin, which tests the first login only,
and a test for repeatedRemoteLogin, which will needs a first login as setup,
and then tests the repeated login only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I think all requests done,
and submitted separate PR for kill method


remote1.remove;
thisProcess.platform.killProcessByID(serverPid);
}

test_repeatedRemoteLogin {

var options, serverPid, remote1, remote2, cond, timer;

options = ServerOptions.new;
options.maxLogins = 4;

// simulate a remote server process by starting a server process independently of SC
serverPid = unixCmd(Server.program + options.asOptionsString);

// login with standard Server.remote method to test for
remote1 = Server.remote(\remoteLogin1, options: options, clientID: 3);
cond = Condition.new;
timer = fork { 3.wait; cond.unhang };

remote1.doWhenBooted {
"% - server first login timed out.\n".postf(thisMethod);
timer.stop;
cond.unhang;
};
cond.hang;

// now login again from a different server object,
// but from the same client address (in the test, local, in reality, it would be remote).
// This simulates a crashed remote client which gets restarted,
// and logs in with its old setup data.

// Because client netaddr is already registered in the server process,
// the response goes thru prHandleNotifyFailString, and prHandleLoginWhenAlreadyRegistered

remote2 = Server.remote(\remoteLogin2, options: options);
cond = Condition.new;
timer = fork { 3.wait; cond.unhang };

remote2.doWhenBooted {
"% - server repeated login timed out.\n".postf(thisMethod);
timer.stop;
cond.unhang;
};

cond.hang;

this.assert(remote2.serverRunning,
"after re-login from a new server object, remote client should be known to be fully running."
);

this.assert(remote2.clientID == 3,
"after re-login from a new server object, remote client should have its requested clientID."
);

// cleanup
remote1.remove;
remote2.remove;
thisProcess.platform.killProcessByID(serverPid);
}
}