NodeProxy fadeTime > 0 leads to hung nodes with Pmono

[frozenbears]

Environment

• SuperCollider version: 3.12.2
• Operating system: macOS 10.13.6
• Other details (Qt version, audio driver, etc.):

Steps to reproduce

Ndef(\foo).fadeTime = 1;
Ndef(\foo).play;
// Toggling between these should cause hung notes to stack up
Ndef(\foo, Pmono(\default, \freq, Pseq([400, 500], inf)))
Ndef(\foo, Pmono(\default, \freq, Pseq([500, 600], inf)))

Expected vs. actual behavior

(Apologies if this has been reported before, I didn't see it when searching through the history, though I did find several past issues with Pmono and other pieces of JITLib.)

My expectation would be that fading between two Ndefs with Pmono would not result in any hung notes. In my initial investigation I noticed that PatternControl calls the xstop extension method to initiate a fadeout using PfadeOut, and then schedules a future stop on the stream player to trigger cleanup:

https://github.com/supercollider/supercollider/blob/develop/SCClassLibrary/JITLib/ProxySpace/ProxyInterfaces.sc#L141
https://github.com/supercollider/supercollider/blob/develop/SCClassLibrary/JITLib/Patterns/extRoutine.sc#L41

From what I can tell tracing through the code, the cleanup function on PmonoStream is simply not getting triggered, which maybe makes sense since PatternControl is just swapping out the stream on the already playing EventStreamPlayer instead of creating a new one and playing it. At the time PmonoStream registers the cleanup function the parent stream doesn't exist yet, and so the cleanup function isn't ever communicated up the hierarchy.

Additional notes

I managed to "fix" this with a crude hack in PatternControl.stopStreams, by making a copy of the event stream players before calling xstop:

stopStreams { | streams, dt |
	var streamsCopy;
	dt = (dt ? fadeTime).value;
	if(dt <= 0.02) {
		streams.do { arg item; item.stop  }
	} {
		dt = dt / clock.beatDur;
		streamsCopy = streams.deepCopy;
		streams.do { arg item; item.postln; item.xstop(dt) };
		// make sure it is stopped, in case next is never called
		SystemClock.sched(dt, { streamsCopy.do(_.stop) });
	}
}

I assume this is probably not the right way to address the problem, and admit to only sort of understanding how EventStreamCleanup is supposed to work in practice.

[telephon]

I think this is a reasonable first step. A potential fix should just go into xstop. There is no need to copy, you just need to make sure that if next is called, the scheduled stop isn't called twice.