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

Server#waitForBoot is broken #3280

Closed
miguel-negrao opened this Issue Nov 5, 2017 · 33 comments

Comments

Projects
None yet
7 participants
@miguel-negrao
Member

miguel-negrao commented Nov 5, 2017

Quick look seems to be due to a1264c2 remove Server:initTree from bootInit..

reproducer:

	Server.local.waitForBoot({
		{ SinOsc.ar(440)*0.1 }.play;
	});

results in

SuperCollider 3 server ready.
Shared memory server interface initialized
FAILURE IN SERVER /s_new Group 1 not found
Requested notification messages from server 'localhost'
localhost: scsynth maxLogins 1 match with my options.
localhost: keeping clientID 0 as confirmed from scsynth.

@adcxyz can you have a look ? :-)

@adcxyz

This comment has been minimized.

Show comment
Hide comment
@adcxyz

adcxyz Nov 6, 2017

Contributor

@miguel-negrao - Thanks for the report!

I am not fully sure how best to resolve this, because waitForBoot and doWhenBooted have two uses:

  1. Low-level methods that handle internal things in server initialisation, which is fine.
  2. They are also used for user code startup code that requires groups to be present
    (such as making a synth onboot).
    These user-code uses should now only happen after scsynth has responded with a valid clientID, because the server is only fully initialised after that.

To distinguish the two cases, we could rename the internal ones to prDoWhenBooted etc;
and change waitForBoot and doWhenBooted so they happen after server init has finished, so they work again as user code interface.
Opinions on this, @jamshark70 @telephon ?

Contributor

adcxyz commented Nov 6, 2017

@miguel-negrao - Thanks for the report!

I am not fully sure how best to resolve this, because waitForBoot and doWhenBooted have two uses:

  1. Low-level methods that handle internal things in server initialisation, which is fine.
  2. They are also used for user code startup code that requires groups to be present
    (such as making a synth onboot).
    These user-code uses should now only happen after scsynth has responded with a valid clientID, because the server is only fully initialised after that.

To distinguish the two cases, we could rename the internal ones to prDoWhenBooted etc;
and change waitForBoot and doWhenBooted so they happen after server init has finished, so they work again as user code interface.
Opinions on this, @jamshark70 @telephon ?

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 6, 2017

Member

yes, I think this would be the right way to go, and would be easy to understand.

Member

telephon commented Nov 6, 2017

yes, I think this would be the right way to go, and would be easy to understand.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 6, 2017

Contributor

Tbh I'm quite lost on the current state, so I don't have much of an opinion except that all of this is hacking, and it will have to be ripped out and rebuilt from the ground up for 3.9.1.

Contributor

jamshark70 commented Nov 6, 2017

Tbh I'm quite lost on the current state, so I don't have much of an opinion except that all of this is hacking, and it will have to be ripped out and rebuilt from the ground up for 3.9.1.

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Nov 6, 2017

Member

all of this is hacking, and it will have to be ripped out and rebuilt from the ground up for 3.9.1.

I haven't been following these discussions very closely, but my totally unrequested opinion is that it seems like there's a lot of "this case is broken, let's add a special patch for it," happening either in parallel with or recursively inside other similar patches. Maybe we should just accept that things are somewhat broken for now and work on a corrected version that we can put into future versions. It may take a bit longer but it will save a ton of technical debt in the future.

Member

brianlheim commented Nov 6, 2017

all of this is hacking, and it will have to be ripped out and rebuilt from the ground up for 3.9.1.

I haven't been following these discussions very closely, but my totally unrequested opinion is that it seems like there's a lot of "this case is broken, let's add a special patch for it," happening either in parallel with or recursively inside other similar patches. Maybe we should just accept that things are somewhat broken for now and work on a corrected version that we can put into future versions. It may take a bit longer but it will save a ton of technical debt in the future.

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 6, 2017

Member

@jamshark70 much of the server code was rebuilt from ground up in the large server refactoring. You should have seen how it looked before. So don't be too negative, or I'll start to cry :).

Note that many issues we currently have were things that actually were broken before, and got exposed by the refactoring.

And yes, rebuilding is probably a good idea for things that are fragile.

Member

telephon commented Nov 6, 2017

@jamshark70 much of the server code was rebuilt from ground up in the large server refactoring. You should have seen how it looked before. So don't be too negative, or I'll start to cry :).

Note that many issues we currently have were things that actually were broken before, and got exposed by the refactoring.

And yes, rebuilding is probably a good idea for things that are fragile.

@snappizz snappizz added this to the 3.9 milestone Nov 6, 2017

@snappizz snappizz added the severe label Nov 6, 2017

@adcxyz

This comment has been minimized.

Show comment
Hide comment
@adcxyz

adcxyz Nov 6, 2017

Contributor

dear @brianlheim & @jamshark70,
I completely understand your skepticism, I know this has not been going smoothly at all ...
There are a lot of assumptions in the server-boot related code, no comprehensive set of tests yet,
and new sloppy spots keep turning up...

Tightening the demands (e.g. waiting for a trustworthy clientID from scsynth) breaks the next brittle thing, like here, it turned out that doWhenBooted actually happened too early, with an initial clientID setup that could still change. IMHO removing that initial clientID setup was the right thing to do, and commit 0dca048 moves in the right direction:
doWhenBooted now happens after server.isFullyBooted.

It still starts independent routines that may happen in any order,
so we should advise users to use only a single call to doWhenBooted or waitForBoot:

s.quit;
s.doWhenBooted({ "doWhenBooted 1".postln; });
s.doWhenBooted({ "doWhenBooted 2".postln; });
s.doWhenBooted({ "doWhenBooted 3".postln; });
s.waitForBoot({ "waitForBoot 4".postln; s.nextNodeID.postln; });
// -> order of execution is random here...

By comparison, ServerTree evaluates its objects in order,
and in initTree it is now called from a Routine, so they can sync and wait if needed.
@jamshark70, is this what you propose for ServerBoot as well?
If yes, I can try and sketch that ASAP.

s.quit;
ServerTree.add({ "ServerTree.add 1".postln; s.sync.postln; }, s);
ServerTree.add({ 1.wait; "ServerTree.add 2".postln }, s);
ServerTree.add({ "ServerTree.add 3".postln }, s);
s.boot;
Contributor

adcxyz commented Nov 6, 2017

dear @brianlheim & @jamshark70,
I completely understand your skepticism, I know this has not been going smoothly at all ...
There are a lot of assumptions in the server-boot related code, no comprehensive set of tests yet,
and new sloppy spots keep turning up...

Tightening the demands (e.g. waiting for a trustworthy clientID from scsynth) breaks the next brittle thing, like here, it turned out that doWhenBooted actually happened too early, with an initial clientID setup that could still change. IMHO removing that initial clientID setup was the right thing to do, and commit 0dca048 moves in the right direction:
doWhenBooted now happens after server.isFullyBooted.

It still starts independent routines that may happen in any order,
so we should advise users to use only a single call to doWhenBooted or waitForBoot:

s.quit;
s.doWhenBooted({ "doWhenBooted 1".postln; });
s.doWhenBooted({ "doWhenBooted 2".postln; });
s.doWhenBooted({ "doWhenBooted 3".postln; });
s.waitForBoot({ "waitForBoot 4".postln; s.nextNodeID.postln; });
// -> order of execution is random here...

By comparison, ServerTree evaluates its objects in order,
and in initTree it is now called from a Routine, so they can sync and wait if needed.
@jamshark70, is this what you propose for ServerBoot as well?
If yes, I can try and sketch that ASAP.

s.quit;
ServerTree.add({ "ServerTree.add 1".postln; s.sync.postln; }, s);
ServerTree.add({ 1.wait; "ServerTree.add 2".postln }, s);
ServerTree.add({ "ServerTree.add 3".postln }, s);
s.boot;
@adcxyz

This comment has been minimized.

Show comment
Hide comment
@adcxyz

adcxyz Nov 6, 2017

Contributor

@jamshark70, is this what you propose for ServerBoot as well?
If yes, I can try and sketch that ASAP.

OK, got it working really simply, see here:
https://github.com/adcxyz/supercollider/tree/doWhenBooted_single_task

Contributor

adcxyz commented Nov 6, 2017

@jamshark70, is this what you propose for ServerBoot as well?
If yes, I can try and sketch that ASAP.

OK, got it working really simply, see here:
https://github.com/adcxyz/supercollider/tree/doWhenBooted_single_task

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

Here's what I think we need to do:

For 3.9.0

I realize we're trying to get 3.9 out the door, but Brian is correct about this: "it seems like there's a lot of 'this case is broken, let's add a special patch for it,' happening either in parallel with or recursively inside other similar patches." One antidote is unit testing. If we had proper unit testing, then the removal of initTree would have been done completely the first time, instead of halfway (and wait for somebody else to find the problem).

  • Define a short list of "canonical" boot scenarios that we expect to work, for instance:

    • s.waitForBoot { ... } -- and use allocators etc.
    • s.boot; s.doWhenBooted { ... }
    • That's probably sufficient. Any other boot cases would be, for now, at the user's own risk.
  • Write unit tests.

  • Strict policy: Absolutely no commits to the server boot process without running the unit tests. Ever. I don't necessarily want to be "strict," but the fact is that we've had way too many "This ought to work" commits.

For 3.9.1

  • More comprehensive list of boot cases.

  • Streamline the logic into one boot thread (perhaps based on the initial attempt at adcxyz@14c17c9). I still tend to think that a rewrite would be better. There's so much cruft left over from band-aids, I don't think polishing in spots will really clean it up.

Contributor

jamshark70 commented Nov 7, 2017

Here's what I think we need to do:

For 3.9.0

I realize we're trying to get 3.9 out the door, but Brian is correct about this: "it seems like there's a lot of 'this case is broken, let's add a special patch for it,' happening either in parallel with or recursively inside other similar patches." One antidote is unit testing. If we had proper unit testing, then the removal of initTree would have been done completely the first time, instead of halfway (and wait for somebody else to find the problem).

  • Define a short list of "canonical" boot scenarios that we expect to work, for instance:

    • s.waitForBoot { ... } -- and use allocators etc.
    • s.boot; s.doWhenBooted { ... }
    • That's probably sufficient. Any other boot cases would be, for now, at the user's own risk.
  • Write unit tests.

  • Strict policy: Absolutely no commits to the server boot process without running the unit tests. Ever. I don't necessarily want to be "strict," but the fact is that we've had way too many "This ought to work" commits.

For 3.9.1

  • More comprehensive list of boot cases.

  • Streamline the logic into one boot thread (perhaps based on the initial attempt at adcxyz@14c17c9). I still tend to think that a rewrite would be better. There's so much cruft left over from band-aids, I don't think polishing in spots will really clean it up.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

Putting onComplete actions into ServerBoot changes the semantics slightly.

  • ServerBoot is persistent. You put an action into it once, and you expect it to run every time the server boots.

  • waitForBoot / doWhenBooted have not been persistent: onComplete applies to this attempt to boot the server only. After quitting the server, the next s.boot (with no action) would not repeat previous onComplete functions.

If I'm reading adcxyz@14c17c9 correctly, waitForBoot has now magically become persistent:

s.waitForBoot { "this should happen only once".postln };

s.quit;

s.boot;

(Hm, ^^ another unit test.)

I know, I know, 3.9 is almost out the door... but we really need to slow down, think more, write less code. We are trying to fix it without understanding it. Result: Basically every move has unintended consequences.

I might regret saying this, but we could hack around the persistence problem by doing something similar to oneShot in the responder suite:

adcxyz@14c17c9#diff-de780da009f27cd5c06ba08d7d17e409R95

ServerBoot.add({ onComplete.value; ServerBoot.remove(onComplete, server) }, server);

(I just checked, AbstractServerAction *performFunction does copy the list of functions before iterating.)

But I repeat, that's a hack, I very strongly dislike it. I mention it primarily to criticize it before someone else suggests it 😉 Instead of differentiating between a persistent list of actions and a temporary queue, we're forcing the persistent list to act like a temporary queue. I would predict future technical debt from that.

Contributor

jamshark70 commented Nov 7, 2017

Putting onComplete actions into ServerBoot changes the semantics slightly.

  • ServerBoot is persistent. You put an action into it once, and you expect it to run every time the server boots.

  • waitForBoot / doWhenBooted have not been persistent: onComplete applies to this attempt to boot the server only. After quitting the server, the next s.boot (with no action) would not repeat previous onComplete functions.

If I'm reading adcxyz@14c17c9 correctly, waitForBoot has now magically become persistent:

s.waitForBoot { "this should happen only once".postln };

s.quit;

s.boot;

(Hm, ^^ another unit test.)

I know, I know, 3.9 is almost out the door... but we really need to slow down, think more, write less code. We are trying to fix it without understanding it. Result: Basically every move has unintended consequences.

I might regret saying this, but we could hack around the persistence problem by doing something similar to oneShot in the responder suite:

adcxyz@14c17c9#diff-de780da009f27cd5c06ba08d7d17e409R95

ServerBoot.add({ onComplete.value; ServerBoot.remove(onComplete, server) }, server);

(I just checked, AbstractServerAction *performFunction does copy the list of functions before iterating.)

But I repeat, that's a hack, I very strongly dislike it. I mention it primarily to criticize it before someone else suggests it 😉 Instead of differentiating between a persistent list of actions and a temporary queue, we're forcing the persistent list to act like a temporary queue. I would predict future technical debt from that.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

FWIW, in the current 3.9 branch (not "develop," not any PR branches), waitForBoot is not persistent, as I thought:

(
ServerBoot.add({ "server boot action".postln; }, s);
s.waitForBoot { "onComplete, should be just once".postln };
)

booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
server boot action
Shared memory server interface initialized
onComplete, should be just once
Requested notification messages from server 'localhost'
localhost: scsynth maxLogins 1 match with my options.
localhost: keeping clientID 0 as confirmed from scsynth.

s.quit;
server 'localhost' disconnected shared memory interface
'/quit' sent

-> localhost
RESULT = 0

s.boot;
booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
server boot action
Shared memory server interface initialized
Requested notification messages from server 'localhost'
localhost: scsynth maxLogins 1 match with my options.
localhost: keeping clientID 0 as confirmed from scsynth.

... where, in the second boot, the "onComplete" message did not print but "server boot action" did.

Let's avoid a semantic change here for 3.9.

Contributor

jamshark70 commented Nov 7, 2017

FWIW, in the current 3.9 branch (not "develop," not any PR branches), waitForBoot is not persistent, as I thought:

(
ServerBoot.add({ "server boot action".postln; }, s);
s.waitForBoot { "onComplete, should be just once".postln };
)

booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
server boot action
Shared memory server interface initialized
onComplete, should be just once
Requested notification messages from server 'localhost'
localhost: scsynth maxLogins 1 match with my options.
localhost: keeping clientID 0 as confirmed from scsynth.

s.quit;
server 'localhost' disconnected shared memory interface
'/quit' sent

-> localhost
RESULT = 0

s.boot;
booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
server boot action
Shared memory server interface initialized
Requested notification messages from server 'localhost'
localhost: scsynth maxLogins 1 match with my options.
localhost: keeping clientID 0 as confirmed from scsynth.

... where, in the second boot, the "onComplete" message did not print but "server boot action" did.

Let's avoid a semantic change here for 3.9.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

Also, I note that ServerBoot happens before shared memory initialization, while onComplete happens after. If the user was counting on doing anything in onComplete that depended on shared memory, it would fail after a proposed change to let ServerBoot handle onComplete actions.

If anything, maybe ServerTree could handle it. But we don't have the unit tests yet, and that wouldn't address the persistence problem.

(
s.waitForBoot {
	var b = Bus(\control, 0, 1, s);
	b.setSynchronous(100);
	0.1.wait;
	b.get { |value| "bus should be 100... %\n".postf(value) };
};
)
booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
Shared memory server interface initialized
bus should be 100... 100  <<--- YES
...

s.quit;

(
var b = Bus(\control, 0, 1, s);
ServerBoot.add({ b.setSynchronous(100) }, s);
s.waitForBoot {
	0.1.wait;
	b.get { |value| "bus should be 100... %\n".postf(value) };
};
)

booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
ERROR: Server-getControlBusValue only supports local servers

PROTECTED CALL STACK:
... omitted

Shared memory server interface initialized
bus should be 100... 0  <<--- OOPS
...
Contributor

jamshark70 commented Nov 7, 2017

Also, I note that ServerBoot happens before shared memory initialization, while onComplete happens after. If the user was counting on doing anything in onComplete that depended on shared memory, it would fail after a proposed change to let ServerBoot handle onComplete actions.

If anything, maybe ServerTree could handle it. But we don't have the unit tests yet, and that wouldn't address the persistence problem.

(
s.waitForBoot {
	var b = Bus(\control, 0, 1, s);
	b.setSynchronous(100);
	0.1.wait;
	b.get { |value| "bus should be 100... %\n".postf(value) };
};
)
booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
Shared memory server interface initialized
bus should be 100... 100  <<--- YES
...

s.quit;

(
var b = Bus(\control, 0, 1, s);
ServerBoot.add({ b.setSynchronous(100) }, s);
s.waitForBoot {
	0.1.wait;
	b.get { |value| "bus should be 100... %\n".postf(value) };
};
)

booting server 'localhost' on address: 127.0.0.1:57110
...
SuperCollider 3 server ready.
JackDriver: max output latency 46.4 ms
ERROR: Server-getControlBusValue only supports local servers

PROTECTED CALL STACK:
... omitted

Shared memory server interface initialized
bus should be 100... 0  <<--- OOPS
...
@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

Last for now: Let me try to write up my understanding of the boot process tonight and post here. Maybe there will be some details to adjust later but currently we're lacking any sort of guidelines on what is or isn't valid.

I was finally able to update to 3.9 today (I had a new piece last week and I was keeping my environment stable). So I might even be able to start contributing code again. But on this topic, I maintain that we need to have a clear idea of the direction before doing more pasta-style coding (throw some code at the wall and see if it sticks).

Contributor

jamshark70 commented Nov 7, 2017

Last for now: Let me try to write up my understanding of the boot process tonight and post here. Maybe there will be some details to adjust later but currently we're lacking any sort of guidelines on what is or isn't valid.

I was finally able to update to 3.9 today (I had a new piece last week and I was keeping my environment stable). So I might even be able to start contributing code again. But on this topic, I maintain that we need to have a clear idea of the direction before doing more pasta-style coding (throw some code at the wall and see if it sticks).

@miguel-negrao

This comment has been minimized.

Show comment
Hide comment
@miguel-negrao

miguel-negrao Nov 7, 2017

Member

Define a short list of "canonical" boot scenarios that we expect to work, for instance:

s.waitForBoot { ... } -- and use allocators etc.
s.boot; s.doWhenBooted { ... }
That's probably sufficient. Any other boot cases would be, for now, at the user's own risk.

Please, also make sure that bootSync keeps working. I always do my initialization after syncing on the server being booted.

Member

miguel-negrao commented Nov 7, 2017

Define a short list of "canonical" boot scenarios that we expect to work, for instance:

s.waitForBoot { ... } -- and use allocators etc.
s.boot; s.doWhenBooted { ... }
That's probably sufficient. Any other boot cases would be, for now, at the user's own risk.

Please, also make sure that bootSync keeps working. I always do my initialization after syncing on the server being booted.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

Please, also make sure that bootSync keeps working. I always do my initialization after syncing on the server being booted.

This kind of feedback is really helpful, thanks! I don't use bootSync myself, though -- could you post a skeleton of the way you use it?

Contributor

jamshark70 commented Nov 7, 2017

Please, also make sure that bootSync keeps working. I always do my initialization after syncing on the server being booted.

This kind of feedback is really helpful, thanks! I don't use bootSync myself, though -- could you post a skeleton of the way you use it?

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

Let me try to write up my understanding of the boot process tonight and post here.

OK... let's see what I can come up with. I'm a bit fuzzy on the details of the server's clientID reply.

Based on printed messages during the startup sequence, in 3.9, we have:

ServerBoot.add { "ServerBoot action" };
ServerTree.add { "ServerTree action" };
s.waitForBoot { "onComplete action".debug };
  1. Start the server process ("booting server 'localhost' on address: 127.0.0.1:57110").
  2. "ServerBoot action"
  3. "Shared memory server interface initialized"
  4. "onComplete action"
  5. "Requested notification messages from server 'localhost'"
  6. "localhost: scsynth maxLogins 1 match with my options. \ localhost: keeping clientID 0 as confirmed from scsynth."
  7. "ServerTree action"

To my knowledge, there aren't any other elements in the boot sequence. bootSync, which Miguel mentioned, uses waitForBoot so it will be included in item 4.

Of these, I think the order of 4, 5, and 6 are negotiable (though, 5 triggers 6 currently in sendNotifyRequest). I'd suggest to change it to the following:

  1. Start the server process ("booting server 'localhost' on address: 127.0.0.1:57110").
  2. "ServerBoot action"
  3. "Shared memory server interface initialized"
  4. "Requested notification messages from server 'localhost'"
  5. "localhost: scsynth maxLogins 1 match with my options. \ localhost: keeping clientID 0 as confirmed from scsynth."
  6. "ServerTree action"
  7. "onComplete action"

... because I can't think of a good reason why onComplete needs to precede notifications, but I can imagine many problem scenarios where an onComplete action creates synths, and then the server reports a different clientID, invalidating the node IDs created by onComplete.

Also, shouldn't we initialize the server tree before doing user init actions? IIRC, the purpose of ServerTree is to allow the user to add to the normal process of creating group 1... so then, the user would likely assume that ServerTree groups already exist. That is, the following looks like it ought to work, but it currently fails.

// effectively, g is an alternate "default group" -- but it isn't available on time
ServerTree.add({ g = Group.new }, s);
s.waitForBoot { a = Synth(\default, [freq: 220], target: g) };

I guess the big challenge in ensuring the right order is that the various actions are split up into different methods, routines or OSCFuncs. Consolidating these into one and only one thread will be essential. Maybe not for 3.9.

Persistence

ServerBoot, ServerTree (and ServerQuit) are persistent. Typical use is to add actions in the startup file, and leave them in place, so you can assume they will happen without paying further attention to them.

waitForBoot, doWhenBooted, and bootSync are not persistent. Actions registered this way should be forgotten when the server boots successfully and if the server fails.

The failure case is actually a remaining problem (which I wouldn't mind working on). If the server process terminates before the client initializes, currently (3.9) the client never recognizes that the server failed, so the actions are never forgotten. Even if we reintroduce the limit condition in doWhenBooted, currently it's 100 tries * 0.2 sec/try = 20 seconds to wait. So the user might do this:

s.waitForBoot { ... };

// failed -- "STATUS = something" posts right away

// user fixes the problem in less than 20 seconds

s.waitForBoot { ... };
// and both user actions fire

IMO only the second of the two waitForBoot actions should take effect.

Did I miss anything?

Contributor

jamshark70 commented Nov 7, 2017

Let me try to write up my understanding of the boot process tonight and post here.

OK... let's see what I can come up with. I'm a bit fuzzy on the details of the server's clientID reply.

Based on printed messages during the startup sequence, in 3.9, we have:

ServerBoot.add { "ServerBoot action" };
ServerTree.add { "ServerTree action" };
s.waitForBoot { "onComplete action".debug };
  1. Start the server process ("booting server 'localhost' on address: 127.0.0.1:57110").
  2. "ServerBoot action"
  3. "Shared memory server interface initialized"
  4. "onComplete action"
  5. "Requested notification messages from server 'localhost'"
  6. "localhost: scsynth maxLogins 1 match with my options. \ localhost: keeping clientID 0 as confirmed from scsynth."
  7. "ServerTree action"

To my knowledge, there aren't any other elements in the boot sequence. bootSync, which Miguel mentioned, uses waitForBoot so it will be included in item 4.

Of these, I think the order of 4, 5, and 6 are negotiable (though, 5 triggers 6 currently in sendNotifyRequest). I'd suggest to change it to the following:

  1. Start the server process ("booting server 'localhost' on address: 127.0.0.1:57110").
  2. "ServerBoot action"
  3. "Shared memory server interface initialized"
  4. "Requested notification messages from server 'localhost'"
  5. "localhost: scsynth maxLogins 1 match with my options. \ localhost: keeping clientID 0 as confirmed from scsynth."
  6. "ServerTree action"
  7. "onComplete action"

... because I can't think of a good reason why onComplete needs to precede notifications, but I can imagine many problem scenarios where an onComplete action creates synths, and then the server reports a different clientID, invalidating the node IDs created by onComplete.

Also, shouldn't we initialize the server tree before doing user init actions? IIRC, the purpose of ServerTree is to allow the user to add to the normal process of creating group 1... so then, the user would likely assume that ServerTree groups already exist. That is, the following looks like it ought to work, but it currently fails.

// effectively, g is an alternate "default group" -- but it isn't available on time
ServerTree.add({ g = Group.new }, s);
s.waitForBoot { a = Synth(\default, [freq: 220], target: g) };

I guess the big challenge in ensuring the right order is that the various actions are split up into different methods, routines or OSCFuncs. Consolidating these into one and only one thread will be essential. Maybe not for 3.9.

Persistence

ServerBoot, ServerTree (and ServerQuit) are persistent. Typical use is to add actions in the startup file, and leave them in place, so you can assume they will happen without paying further attention to them.

waitForBoot, doWhenBooted, and bootSync are not persistent. Actions registered this way should be forgotten when the server boots successfully and if the server fails.

The failure case is actually a remaining problem (which I wouldn't mind working on). If the server process terminates before the client initializes, currently (3.9) the client never recognizes that the server failed, so the actions are never forgotten. Even if we reintroduce the limit condition in doWhenBooted, currently it's 100 tries * 0.2 sec/try = 20 seconds to wait. So the user might do this:

s.waitForBoot { ... };

// failed -- "STATUS = something" posts right away

// user fixes the problem in less than 20 seconds

s.waitForBoot { ... };
// and both user actions fire

IMO only the second of the two waitForBoot actions should take effect.

Did I miss anything?

@brianlheim

This comment has been minimized.

Show comment
Hide comment
@brianlheim

brianlheim Nov 7, 2017

Member

dear @brianlheim & @jamshark70,
I completely understand your skepticism, I know this has not been going smoothly at all ...

No worries! I hope I'm not adding to your stress. If anything, I wanted to express my encouragement for continuing work on this, just in a direction that will be more productive in the long run. I am very appreciative that you're spending the time you are on it! :)

Member

brianlheim commented Nov 7, 2017

dear @brianlheim & @jamshark70,
I completely understand your skepticism, I know this has not been going smoothly at all ...

No worries! I hope I'm not adding to your stress. If anything, I wanted to express my encouragement for continuing work on this, just in a direction that will be more productive in the long run. I am very appreciative that you're spending the time you are on it! :)

@miguel-negrao

This comment has been minimized.

Show comment
Hide comment
@miguel-negrao

miguel-negrao Nov 7, 2017

Member

Please, also make sure that bootSync keeps working. I always do my initialization after syncing on the server being booted.

This kind of feedback is really helpful, thanks! I don't use bootSync myself, though -- could you post a skeleton of the way you use it?

Sure:

(
Routine({
	Server.local.bootSync;
	{ SinOsc.ar(440)*0.1 }.play;
}).play(AppClock);
)

I use Routine with AppClock because usually I also do some GUI stuff in that function.

Member

miguel-negrao commented Nov 7, 2017

Please, also make sure that bootSync keeps working. I always do my initialization after syncing on the server being booted.

This kind of feedback is really helpful, thanks! I don't use bootSync myself, though -- could you post a skeleton of the way you use it?

Sure:

(
Routine({
	Server.local.bootSync;
	{ SinOsc.ar(440)*0.1 }.play;
}).play(AppClock);
)

I use Routine with AppClock because usually I also do some GUI stuff in that function.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 7, 2017

Contributor

I think it would be very helpful if somebody could come up with a spec of what is supposed to happen in boot and in what order. My guess is that nobody actually knows in fine detail, so this may be a de facto proposal! ;-)

In doing this, it would be good if we could make clear or define the useful distinctions between the various stages and interfaces, i.e. what's the idea of waitForBoot vs. doWhenBooted vs. bootSync vs. ServerBoot vs. ServerTree, etc. In cases where there's no clear reason for a distinction would it make sense to simplify and collapse some of these? e.g. maybe waitForBoot and doWhenBooted should just be convenience front ends to ServerBoot.

The main thing is a clear, sensible, comprehensible and documented sequence. With that it will be clearer what to do, and provide a benchmark for UnitTests and future changes.

Apologies if that already exists, but if it does I don't think I've ever seen it. :-)

Contributor

muellmusik commented Nov 7, 2017

I think it would be very helpful if somebody could come up with a spec of what is supposed to happen in boot and in what order. My guess is that nobody actually knows in fine detail, so this may be a de facto proposal! ;-)

In doing this, it would be good if we could make clear or define the useful distinctions between the various stages and interfaces, i.e. what's the idea of waitForBoot vs. doWhenBooted vs. bootSync vs. ServerBoot vs. ServerTree, etc. In cases where there's no clear reason for a distinction would it make sense to simplify and collapse some of these? e.g. maybe waitForBoot and doWhenBooted should just be convenience front ends to ServerBoot.

The main thing is a clear, sensible, comprehensible and documented sequence. With that it will be clearer what to do, and provide a benchmark for UnitTests and future changes.

Apologies if that already exists, but if it does I don't think I've ever seen it. :-)

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor

I think it would be very helpful if somebody could come up with a spec of what is supposed to happen in boot and in what order.

Chuckling a bit because I made a stab at that, literally 4-5 hours before you wrote this... scroll up a few messages 😉

Contributor

jamshark70 commented Nov 7, 2017

I think it would be very helpful if somebody could come up with a spec of what is supposed to happen in boot and in what order.

Chuckling a bit because I made a stab at that, literally 4-5 hours before you wrote this... scroll up a few messages 😉

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 7, 2017

Contributor

@jamshark70 yeah I did see that. Still missing the complete rationale, (and possible rationalisation!)

:-)

Contributor

muellmusik commented Nov 7, 2017

@jamshark70 yeah I did see that. Still missing the complete rationale, (and possible rationalisation!)

:-)

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 7, 2017

Contributor
(
Routine({
	Server.local.bootSync;
	{ SinOsc.ar(440)*0.1 }.play;
}).play(AppClock);
)

I think it will be fine -- bootSync signals the condition in a waitForBoot action, so it would happen in the "onComplete" step. Breaking this would mean that onComplete actions could somehow magically be dropped, which IMO would be automatically invalid.

But, a question: My proposal for 3.9.1 is to put all onComplete actions into a queue, to guarantee order. Currently, these already run in the context of routines. Would you like your bootSync'ed routine to be synchronous or asynchronous WRT other boot actions?

I'm going to propose making them synchronous. If you do a waitForBoot (A), a bootSync (B) and another waitForBoot (C), I think A should 100% finish before B, which finishes before C.

Currently they are in separate, unsynced threads. The order of execution may not be predictable.

Contributor

jamshark70 commented Nov 7, 2017

(
Routine({
	Server.local.bootSync;
	{ SinOsc.ar(440)*0.1 }.play;
}).play(AppClock);
)

I think it will be fine -- bootSync signals the condition in a waitForBoot action, so it would happen in the "onComplete" step. Breaking this would mean that onComplete actions could somehow magically be dropped, which IMO would be automatically invalid.

But, a question: My proposal for 3.9.1 is to put all onComplete actions into a queue, to guarantee order. Currently, these already run in the context of routines. Would you like your bootSync'ed routine to be synchronous or asynchronous WRT other boot actions?

I'm going to propose making them synchronous. If you do a waitForBoot (A), a bootSync (B) and another waitForBoot (C), I think A should 100% finish before B, which finishes before C.

Currently they are in separate, unsynced threads. The order of execution may not be predictable.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 8, 2017

Contributor

Still missing the complete rationale...

Well, it was 11:30 at night when I posted it...

Anyway, the user has influence over ServerBoot, ServerTree and onComplete (added by waitForBoot/doWhenBooted).

  • ServerBoot. I don't know why we need this and I wouldn't mind deleting it. I can't think of a single case where you need to do something before shared memory init, where it would be wrong to do it either totally before boot or after init.

  • ServerTree AFAIK is for setup that you want to do every time you boot in an sclang session: set it and forget it, and it'll just be there. A good case might be a class that requires something to exist in the server: its initClass does ServerTree.add(...) and the user is then not responsible for remembering to init that class at boot time. This seems sensible because it happens at the end of the boot sequence (unlike ServerBoot).

  • onComplete actions are for specific instances of booting for a particular piece or application. As specific instances, these actions should either run on success, or be forgotten on failure.

I'd argue that ServerTree is general and onComplete is specific, so onComplete should be last. That isn't currently the case.

Contributor

jamshark70 commented Nov 8, 2017

Still missing the complete rationale...

Well, it was 11:30 at night when I posted it...

Anyway, the user has influence over ServerBoot, ServerTree and onComplete (added by waitForBoot/doWhenBooted).

  • ServerBoot. I don't know why we need this and I wouldn't mind deleting it. I can't think of a single case where you need to do something before shared memory init, where it would be wrong to do it either totally before boot or after init.

  • ServerTree AFAIK is for setup that you want to do every time you boot in an sclang session: set it and forget it, and it'll just be there. A good case might be a class that requires something to exist in the server: its initClass does ServerTree.add(...) and the user is then not responsible for remembering to init that class at boot time. This seems sensible because it happens at the end of the boot sequence (unlike ServerBoot).

  • onComplete actions are for specific instances of booting for a particular piece or application. As specific instances, these actions should either run on success, or be forgotten on failure.

I'd argue that ServerTree is general and onComplete is specific, so onComplete should be last. That isn't currently the case.

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 8, 2017

Member

I agree we could get rid of ServerBoot if we add all these inits (like SynthDescLib, NodeWatcheretc.) in ServerTree in the correct order. It might be good to alias the name of the class, keeping ServerTree, but using ServerSetup (?) everywhere in the class library.

Member

telephon commented Nov 8, 2017

I agree we could get rid of ServerBoot if we add all these inits (like SynthDescLib, NodeWatcheretc.) in ServerTree in the correct order. It might be good to alias the name of the class, keeping ServerTree, but using ServerSetup (?) everywhere in the class library.

@miguel-negrao

This comment has been minimized.

Show comment
Hide comment
@miguel-negrao

miguel-negrao Nov 8, 2017

Member

But, a question: My proposal for 3.9.1 is to put all onComplete actions into a queue, to guarantee order. Currently, these already run in the context of routines. Would you like your bootSync'ed routine to be synchronous or asynchronous WRT other boot actions?

I'm going to propose making them synchronous. If you do a waitForBoot (A), a bootSync (B) and another waitForBoot (C), I think A should 100% finish before B, which finishes before C.

Currently they are in separate, unsynced threads. The order of execution may not be predictable.

For my use case it doesn't matter, I have all my actions after a single bootSync. Feel free to choose what you find most appropriate.

Member

miguel-negrao commented Nov 8, 2017

But, a question: My proposal for 3.9.1 is to put all onComplete actions into a queue, to guarantee order. Currently, these already run in the context of routines. Would you like your bootSync'ed routine to be synchronous or asynchronous WRT other boot actions?

I'm going to propose making them synchronous. If you do a waitForBoot (A), a bootSync (B) and another waitForBoot (C), I think A should 100% finish before B, which finishes before C.

Currently they are in separate, unsynced threads. The order of execution may not be predictable.

For my use case it doesn't matter, I have all my actions after a single bootSync. Feel free to choose what you find most appropriate.

@adcxyz

This comment has been minimized.

Show comment
Hide comment
@adcxyz

adcxyz Nov 8, 2017

Contributor

The long ordered spec list is for 3.9.1, right?
Then I believe the issue here (including @miguel-negrao 's bootSync request)
is solved with #3275, with added unit tests for waitForBoot and bootSync to make sure.
gurus please review and verify ...

Contributor

adcxyz commented Nov 8, 2017

The long ordered spec list is for 3.9.1, right?
Then I believe the issue here (including @miguel-negrao 's bootSync request)
is solved with #3275, with added unit tests for waitForBoot and bootSync to make sure.
gurus please review and verify ...

@miguel-negrao

This comment has been minimized.

Show comment
Hide comment
@miguel-negrao

miguel-negrao Nov 8, 2017

Member

#3275 solves the issue over here.

Member

miguel-negrao commented Nov 8, 2017

#3275 solves the issue over here.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 8, 2017

Contributor

Sorry, busy day so took me a while to get back to this.

ServerBoot. I don't know why we need this and I wouldn't mind deleting it. I can't think of a single case where you need to do something before shared memory init, where it would be wrong to do it either totally before boot or after init.

I can't see why it should happen before shared memory is inited, but I'm not sure it's unneeded.

It seems to me the primary distinction between ServerBoot and ServerTree is that the first happens only at boot, and the second every time the tree is rebuilt, i.e. after every Cmd-. (and after CmdPeriod runs; this should also be documented). ServerTree differs from CmdPeriod in that it guarantees that the default group exists. As well it is not to my understanding only 'for setup that you want to do every time you boot in an sclang session', but is very useful for specific pieces, setups and in classes.

I can't off the top of my head think of a use for ServerBoot, i.e. something to do only when booting, but not each time the tree is built, but it is not obvious to me that that is a nonsensical thing to want to do. There might be things you need to do only if a server went down and needed restarting in a long running installation for instance.

It also shares an interface with the other AbstractSystemActions, which is thus easily learnable.

Persistence

CmdPeriod has doOnce, other AbstractSystemActions do not. Why? I think it's reasonable to have these at least in the ServerAction classes, especially ServerTree. This leads me to...

I'd argue that ServerTree is general and onComplete is specific, so onComplete should be last. That isn't currently the case.

I think it is worth considering whether these need to be distinct. With a *doOnce, ServerTree becomes potentially specific. What is currently onComplete could simply use ServerTree:doOnce instead of being a separate step (which needs to be understood). The primary issue is control of ordering. I can't think of a situation where one couldn't order things correctly in ServerTree (just add them in order, or use one if need be), but I could be wrong.

As a related issue, the startup (StartUp...) and shutdown sequences should also be documented.

Contributor

muellmusik commented Nov 8, 2017

Sorry, busy day so took me a while to get back to this.

ServerBoot. I don't know why we need this and I wouldn't mind deleting it. I can't think of a single case where you need to do something before shared memory init, where it would be wrong to do it either totally before boot or after init.

I can't see why it should happen before shared memory is inited, but I'm not sure it's unneeded.

It seems to me the primary distinction between ServerBoot and ServerTree is that the first happens only at boot, and the second every time the tree is rebuilt, i.e. after every Cmd-. (and after CmdPeriod runs; this should also be documented). ServerTree differs from CmdPeriod in that it guarantees that the default group exists. As well it is not to my understanding only 'for setup that you want to do every time you boot in an sclang session', but is very useful for specific pieces, setups and in classes.

I can't off the top of my head think of a use for ServerBoot, i.e. something to do only when booting, but not each time the tree is built, but it is not obvious to me that that is a nonsensical thing to want to do. There might be things you need to do only if a server went down and needed restarting in a long running installation for instance.

It also shares an interface with the other AbstractSystemActions, which is thus easily learnable.

Persistence

CmdPeriod has doOnce, other AbstractSystemActions do not. Why? I think it's reasonable to have these at least in the ServerAction classes, especially ServerTree. This leads me to...

I'd argue that ServerTree is general and onComplete is specific, so onComplete should be last. That isn't currently the case.

I think it is worth considering whether these need to be distinct. With a *doOnce, ServerTree becomes potentially specific. What is currently onComplete could simply use ServerTree:doOnce instead of being a separate step (which needs to be understood). The primary issue is control of ordering. I can't think of a situation where one couldn't order things correctly in ServerTree (just add them in order, or use one if need be), but I could be wrong.

As a related issue, the startup (StartUp...) and shutdown sequences should also be documented.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 8, 2017

Contributor

Couple of quick comments before I have to race off to work:

  • Yes, the long ordered list is for 3.9.1.

  • So, we should probably merge the fix, close this issue, and open a new issue to plan an improved server init sequence.

  • Scott has a good point -- I had missed the distinction between boot init and cmd-. reinit. So we can't get rid of ServerBoot after all.

  • User boot actions are very likely to depend on at least the default group. So, we can't put them into ServerBoot (because that comes before ServerTree).

  • "What is currently onComplete could simply use ServerTree:doOnce instead of being a separate step (which needs to be understood)" -- This is assuming that the server boots successfully. If it fails, then what is currently onComplete needs to be removed from ServerTree. That sounds breakable.

Contributor

jamshark70 commented Nov 8, 2017

Couple of quick comments before I have to race off to work:

  • Yes, the long ordered list is for 3.9.1.

  • So, we should probably merge the fix, close this issue, and open a new issue to plan an improved server init sequence.

  • Scott has a good point -- I had missed the distinction between boot init and cmd-. reinit. So we can't get rid of ServerBoot after all.

  • User boot actions are very likely to depend on at least the default group. So, we can't put them into ServerBoot (because that comes before ServerTree).

  • "What is currently onComplete could simply use ServerTree:doOnce instead of being a separate step (which needs to be understood)" -- This is assuming that the server boots successfully. If it fails, then what is currently onComplete needs to be removed from ServerTree. That sounds breakable.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 9, 2017

Contributor

But, while I'm thinking about it, and while I'm on the train:

ServerTree happens every time the tree is rebuilt, i.e. after every Cmd-. (and after CmdPeriod runs...)

Do we need both, then? If we're talking about folding onComplete functions into ServerTree (with doOnce and "the right order"), then we could also fold ServerTree into CmdPeriod (in "the right order"). That's a bit of devil's advocacy, intended to test the limits of where we do and don't want to consolidate.

I have a crazy idea: What about naming system actions? I'm concerned about duplicate onComplete's being added if booting fails. Naming them could handle that.

Contributor

jamshark70 commented Nov 9, 2017

But, while I'm thinking about it, and while I'm on the train:

ServerTree happens every time the tree is rebuilt, i.e. after every Cmd-. (and after CmdPeriod runs...)

Do we need both, then? If we're talking about folding onComplete functions into ServerTree (with doOnce and "the right order"), then we could also fold ServerTree into CmdPeriod (in "the right order"). That's a bit of devil's advocacy, intended to test the limits of where we do and don't want to consolidate.

I have a crazy idea: What about naming system actions? I'm concerned about duplicate onComplete's being added if booting fails. Naming them could handle that.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 9, 2017

Contributor

Do we need both, then? If we're talking about folding onComplete functions into ServerTree (with doOnce and "the right order"), then we could also fold ServerTree into CmdPeriod (in "the right order"). That's a bit of devil's advocacy, intended to test the limits of where we do and don't want to consolidate.

IIRC the reason we have both is that we realised that at least in terms of the server, you might well need a separate cleanup stage (Cmd-. is stop) and then a rebuild stage, for the tree. Again I'm not sure that all cases could be simplified so as to avoid that.

I have a crazy idea: What about naming system actions? I'm concerned about duplicate onComplete's being added if booting fails. Naming them could handle that.

Okay, so it's a good point about one shot actions being potentially duplicated if the server fails to boot. It seems to me that this is largely a problem for waitForBoot as you are using that explicitly to boot the server, whereas in other cases it's more likely setup code that is separate from the boot call. It's basically the old side effect of running a block of code twice problem.

So one possible solution: onComplete uses ServerTree, but hangs onto the (doOnce wrapped) func, and removes it from ServerTree on timeout. This is simple. For setup uses, which might call ServerTree:doOnce directly, we would document that users should do the same if that could be an issue. I also like this as it makes a stronger case for the additional interface.

Other solutions seem more awkwardly elaborate to me, e.g. have ServerTree keep track of doOnce objects and dump them on boot fail. I really don't like this as it might not be what you want in all cases.

Actually the way I use OSCdefs mostly is for cases like this, i.e. they're in a block that gets re-executed and thus replaced. We could have a Bootdef, but I sort of don't like that as it's adding 'yet another' interface to the same functionality...

Contributor

muellmusik commented Nov 9, 2017

Do we need both, then? If we're talking about folding onComplete functions into ServerTree (with doOnce and "the right order"), then we could also fold ServerTree into CmdPeriod (in "the right order"). That's a bit of devil's advocacy, intended to test the limits of where we do and don't want to consolidate.

IIRC the reason we have both is that we realised that at least in terms of the server, you might well need a separate cleanup stage (Cmd-. is stop) and then a rebuild stage, for the tree. Again I'm not sure that all cases could be simplified so as to avoid that.

I have a crazy idea: What about naming system actions? I'm concerned about duplicate onComplete's being added if booting fails. Naming them could handle that.

Okay, so it's a good point about one shot actions being potentially duplicated if the server fails to boot. It seems to me that this is largely a problem for waitForBoot as you are using that explicitly to boot the server, whereas in other cases it's more likely setup code that is separate from the boot call. It's basically the old side effect of running a block of code twice problem.

So one possible solution: onComplete uses ServerTree, but hangs onto the (doOnce wrapped) func, and removes it from ServerTree on timeout. This is simple. For setup uses, which might call ServerTree:doOnce directly, we would document that users should do the same if that could be an issue. I also like this as it makes a stronger case for the additional interface.

Other solutions seem more awkwardly elaborate to me, e.g. have ServerTree keep track of doOnce objects and dump them on boot fail. I really don't like this as it might not be what you want in all cases.

Actually the way I use OSCdefs mostly is for cases like this, i.e. they're in a block that gets re-executed and thus replaced. We could have a Bootdef, but I sort of don't like that as it's adding 'yet another' interface to the same functionality...

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon Nov 9, 2017

Member

We could have a Bootdef, but I sort of don't like that as it's adding 'yet another' interface to the same functionality...

If it would help, we could pass a symbol as second argument these add methods, and register every action under a name.

Member

telephon commented Nov 9, 2017

We could have a Bootdef, but I sort of don't like that as it's adding 'yet another' interface to the same functionality...

If it would help, we could pass a symbol as second argument these add methods, and register every action under a name.

@jamshark70

This comment has been minimized.

Show comment
Hide comment
@jamshark70

jamshark70 Nov 9, 2017

Contributor

IIRC the reason we have both is that we realised that at least in terms of the server, you might well need a separate cleanup stage (Cmd-. is stop) and then a rebuild stage, for the tree.

Good reason.

Okay, so it's a good point about one shot actions being potentially duplicated if the server fails to boot.

It took me a good 3-4 tries before anybody noticed ;)

If it would help, we could pass a symbol as second argument these add methods, and register every action under a name.

It would have to be a third argument (the second argument is already the server object), but I was thinking something kind of like this. If the user doesn't give a name, one could be assigned automatically.

Contributor

jamshark70 commented Nov 9, 2017

IIRC the reason we have both is that we realised that at least in terms of the server, you might well need a separate cleanup stage (Cmd-. is stop) and then a rebuild stage, for the tree.

Good reason.

Okay, so it's a good point about one shot actions being potentially duplicated if the server fails to boot.

It took me a good 3-4 tries before anybody noticed ;)

If it would help, we could pass a symbol as second argument these add methods, and register every action under a name.

It would have to be a third argument (the second argument is already the server object), but I was thinking something kind of like this. If the user doesn't give a name, one could be assigned automatically.

@muellmusik

This comment has been minimized.

Show comment
Hide comment
@muellmusik

muellmusik Nov 9, 2017

Contributor

If it would help, we could pass a symbol as second argument these add methods, and register every action under a name.

It would have to be a third argument (the second argument is already the server object), but I was thinking something kind of like this. If the user doesn't give a name, one could be assigned automatically.

Hmm. There are lots of places in the lib where you need to hold a reference to a function if you want to remove/deregister it. That doesn’t seem too onerous, and personally I’d prefer sticking with that to adding a new convention in this one place. If onCompletes timed out I think this would almost never be needed anyway.

Contributor

muellmusik commented Nov 9, 2017

If it would help, we could pass a symbol as second argument these add methods, and register every action under a name.

It would have to be a third argument (the second argument is already the server object), but I was thinking something kind of like this. If the user doesn't give a name, one could be assigned automatically.

Hmm. There are lots of places in the lib where you need to hold a reference to a function if you want to remove/deregister it. That doesn’t seem too onerous, and personally I’d prefer sticking with that to adding a new convention in this one place. If onCompletes timed out I think this would almost never be needed anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment