Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

class library: fix possible sync problem in NodeProxy clear #2845

Merged

Conversation

telephon
Copy link
Member

MixedBundle allows to schedule a function when sending happens. This
is what was used to schedule into the future the clearing of the old
group. It is much better to just delegate this to the server. Because
group objects are registered by NodeProxy with the NodeWatcher, it is
also released correctly. ( NodeWatcher.newFrom(s).nodes).

We need to add a very small offset between the bundle timestamps, so their order is guaranteed on the server.

`MixedBundle` allows to schedule a function when sending happens. This
is what was used to schedule into the future the clearing of the old
group. It is much better to just delegate this to the server. Because
group  objects are registered by NodeProxy with the NodeWatcher, it is
also released correctly. ( `NodeWatcher.newFrom(s).nodes`).

We need to add a very small offset between the bundle timestamps, so their order is guaranteed on the server.
@telephon
Copy link
Member Author

(ping)

1 similar comment
@telephon
Copy link
Member Author

telephon commented May 5, 2017

(ping)

@jamshark70
Copy link
Contributor

I did test this a while ago -- sorry I didn't report back. I'm afraid I don't remember the exact result. I suppose it was successful -- if I had seen a problem, I would have reported it at that time.

I just retested found that I'm not getting an error from clearing a NodeProxy -- so I think the fix is good.

BTW retesting was a bit harder than it needed to be. Unfortunately this branch is now incompatible with the current yaml-cpp submodule, so:

$ git checkout telephon-topic-fix-nodeproxy-clear-timing 
error: The following untracked working tree files would be overwritten by checkout:
	external_libraries/yaml-cpp/.clang-format
	external_libraries/yaml-cpp/.codedocs
	external_libraries/yaml-cpp/.gitignore
	external_libraries/yaml-cpp/.travis.yml
Please move or remove them before you can switch branches.
Aborting

So what might have been just a couple of minutes -- switch branches, test, switch back -- became "try a couple of ways to stash some files that I'm not sure will be restored later because they are untracked," switch branches, test, switch back, discover that the four files didn't come back, and hope it doesn't make a difference the next time I have to build.

@telephon
Copy link
Member Author

telephon commented May 6, 2017

@jamshark70 yes, I stumble over it all the time: git has no very good way to deal with added submodules.

In some other pull request, @bagong showed me a way to switch branches correctly in such a situation, but I can't find it anymore now.

@bagong
Copy link
Contributor

bagong commented May 6, 2017

Yea, failing long time memory is one of the worst problems in command line oriented work (almost as bad as in coding ;) ) I spend most time switching between Google, Stackoverflow and the command line 😄. I think in this case the trick to make it memorizable is to think of a submodules as individual units that can be switched on and off. You address a single submodule by its path. Switch it on with init and switch it off with deinit. And get help with git help submodule. So if you switch to a branch where submodules and folder might conflict, switch the submodule off first...
The problem that can co-occur in this context is that you might need a clean build after a switch. clean to be taken literally hear: to make clean or cmake build . --target clean before switching. That will remove precompiled stuff that still expects the old code (haha, vaguely..). Deleting the build folder is possible of course too, but you will loose your build configuration in that case, which could be anything between irrelevant and pretty destructive...

@telephon
Copy link
Member Author

(can be merged IMHO)

@mossheim
Copy link
Contributor

If someone can give me some test code, I can test it out.

@jamshark70
Copy link
Contributor

s.boot;

Ndef(\x, { SinOsc.ar(440) });

Ndef(\x).clear;  // node not found happens here

@mossheim
Copy link
Contributor

Ah, i didn't realize until i read through #2818 that this is a Linux issue. I don't see that error message on this branch or master on macOS Sierra. Someone on linux should test too.

@telephon
Copy link
Member Author

If I understood this correctly, this is a potential issue on other systems as well?

@telephon
Copy link
Member Author

it's just a little lot hard to reproduce

@gusano
Copy link
Member

gusano commented May 28, 2017

@telephon @jamshark70 this fixes the issue for me on linux (3.9dev, 4.10.6-1-ARCH)

var oldGroup = group;
if(this.isPlaying) {
bundle = MixedBundle.new;
if(fadeTime.notNil) { bundle.add([15, group.nodeID, "fadeTime", fadeTime]) };
if(fadeTime.notNil) {
Copy link
Member

Choose a reason for hiding this comment

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

@telephon A trailing whitespace! : )

Copy link
Member Author

Choose a reason for hiding this comment

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

:)

@telephon
Copy link
Member Author

[OT] I'd like to have http://www.cplusplus.com/reference/cmath/nextafter/ so we can just push up the time by a minimal timestep, because OSC doesn't support order apart from time (or bundles)

@gusano gusano merged commit 2a5e087 into supercollider:master May 28, 2017
@gusano
Copy link
Member

gusano commented May 28, 2017

Danke @telephon !

@telephon telephon deleted the topic-fix-nodeproxy-clear-timing-02 branch May 28, 2017 14:47
@telephon
Copy link
Member Author

you're welcome !

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

Successfully merging this pull request may close these issues.

None yet

5 participants