Topic/fix node proxy free bus timing #4493
Conversation
This method was added in refactoring the method NodeProxy freeBus. It makes clearer what the intention is and encapsulates the slightly complicated scheduling process. The tests cover the most common cases.
Make sure that NodeProxy reshaping frees its old bus in time.
late. The method `schedAfterFade ` was added in refactoring, it makes clearer what the intention is and encapsulates the slightly complicated scheduling process.
We can't remove a function from CmdPeriod here, because `doOnce` wraps it in another function which is not found when calling `remove(func)`. So instead, we just set the function to nil which is simpler anyhow.
telephon
added
bug
|
@adcxyz thanks! does it look ok now? |
This is possible because `0.nextTimeOnGrid(clock) == clock.beats`.
| @@ -127,7 +127,7 @@ TestNodeProxy_Server : UnitTest { | |||
| 0.2.wait; | |||
| proxy.schedAfterFade { ok = true }; | |||
| (proxy.fadeTime + 1.5 + proxy.server.latency - 0.2).wait; | |||
| this.assert(ok, "schedAfterFade should have happened after quant and fadeTime"); | |||
| this.assert(ok, "schedAfterFade should have happen right after quant and fadeTime"); | |||
There was a problem hiding this comment.
still a "have" too many here:
schedAfterFade should happen right after ...
|
@jamshark70 have your requests been addressed? The github interface doesn't work properly, as it seems. |
|
Seems ok to me. |
| usedClock = clock ? TempoClock.default; | ||
| delta = server.latency ? 0.01 + this.fadeTime; // 0.01 is reasonable for a local server | ||
| delta = delta + (quant ? 0).nextTimeOnGrid(usedClock); | ||
| usedClock.schedAbs(delta, { func.value; func = nil }); |
There was a problem hiding this comment.
Being fussy, but why not like this? delta's handling was more complex in an earlier version of this PR, but now it's simpler. Also, for legibility, this is schedAfterFade so perhaps move fadeTime to the front.
usedClock.schedAbs(
this.fadeTime + (server.latency ? 0.01) + (quant ? 0).nextTimeOnGrid(usedClock),
{ func.value; func = nil }
);
|
@jamshark70 thanks for the comment! I hope it's better readable now. |
|
All good for me -- I think we can pull the trigger. |
|
original post says there are breaking changes in this PR -- does someone have a list of what broke, for changelog reasons? |
|
I don't see any breaking changes in this commit anymore. |
Purpose and Motivation
As it turned out, when dynamically changing the number of channels and/or the rate of a NodeProxy, the old bus was freed too late. This occurred for those who use the
reshaping = \elasticmode. After some time this memory leak would lead to a failure of bus allocation due to overrun.The bug was due to a wrong use of
schedinstead ofschedAbs.Some refactoring improved readability and better overall behavior.
Types of changes
To-do list