Generalized conceptual design flaw (double free) of cleanups?

Yep, that’s actually an even more basic problem than the bypassing of the interface that PmonoArtic does. Currently Pmono breaks (on stop) too because of the flag not getting passed to the actual function stored in Thunk…

Thunk : AbstractFunction {

	value {
		^value ?? { value = function.value; function = nil; value }
	}

Yay.

Your Thunk variant/improvement fixes that, of course.


The stuff that is still off somewhat is this line in PmonoArticStream (L132), which is now ineffective because of the Thunks:

event[\removeFromCleanup] = event[\removeFromCleanup].add(currentCleanupFunc);

The effect of that isn’t too dramatic, but it basically makes PmonoArtic with legato < 1 behave like Pmono on stop now (with the Thunk-based cleanup).

(fork {
	p = PmonoArtic(\default, \dur, 2).play;
	2.2.wait;
	p.stop;
})
// vs
(fork {
	p = Pbind(\instrument, \default, \dur, 2).play;
	2.2.wait;
	p.stop;
})

I personally don’t mind that and would like to see it as an option in PmonoArtic if we’re changing that code anyway. But if you want back the Pbind-like behavior on stop in PmonoArtic (for legato < 1), there needs to be an interface for removing stuff from cleanup which now looks-up into thunks. Note that creating another Thunk there will not work, as these are not unique. Probably use an ObjectTable for reverse lookups or some such.

As it turns out, every single call to cleanup.addFunction in classlib ignores the returned value so… you could change the semantics to return the Thunk created, rather then the EventStreamCleanup object as it (implicitly) does now.

+ EventStreamCleanup {

	addFunction { |event, function |
		if(event.isKindOf(Dictionary)) {
			function = CleanupThunk(function); // hjh
			functions.add(function);
			event[\addToCleanup] = event[\addToCleanup].add(function);
		};
        ^function; // new return-value semantics
	}
}

That helps because then in PmonoStream, the stuff that’s stored could be changed (L71) from

    cleanup.addFunction(event, currentCleanupFunc = { | flag |

to

    currentCleanupFunc = cleanup.addFunction(event, { | flag |

So then the aforemention “lookup” in PmonoArticStream (L132) will automagically work I think because it will add the exact same Thunk to the removeFromCleanup that was added to the addToCleanup. I’ve just tried this and it works! Also, the unit test still passes with it. But I still want a configurable PmonoArtic, LOL.

I was actually testing exactly this at about the same time you were posting it here.

Would it be ok, btw, if we stick to github as much as possible for this? I know you experienced an outage, but I’m strapped for time and watching two places for info is not quite feasible for me (and responding twice, in two places, is definitely not going to happen).

hjh

Github is actually working for me now too (again).