Generalized conceptual design flaw (double free) of cleanups?

Questions
1

I thought that only Pproto has this bug, but it’s more widespread:

p = Pfset({}, Pbind(\dur, Pseq([1], 1)), { "cute!".postln })
p.play // cute!

p = Pbind() <> Pfset({}, Pbind(\dur, Pseq([1], 1)), { "cute!".postln })
p.play // two times cute!

p = Pbind() <> Pbind() <> Pfset({}, Pbind(\dur, Pseq([1], 1)), { "cute!".postln })
p.play // not thrice though!

The architecture of the EventStreamCleanup seems rather troubled. Basically an EventStreamCleanup instance keeps a local copy of the cleanups in its functions array and also “sends a copy downstream”, or more precisely copies individual cleanup messages into an Event-serialized form (\addToCleanup), so that a downstream consumers (with the EventStreamPlayer typically being the final consumer) can also keep copies.

Consequently there are lotsa copies of cleanups in a stream chain under some conditions. (Pchain always keeps a local copy for instance because any of its sub-streams can decide to call it quits.) The problem with this design is that any Pattern can introduce downstream-transmission (obviously) and cleanup-execution bugs.

Basically, methinks a cleanup generated once should not be executed several times. But because a lot of streams keep copies, this can happen if one is not careful with the “sharing of responsibilities” when Patterns classes are written.

Since EventStreamCleanup is in such widespread use throughout the Pattern library, I think it would be quite hard to fix the conceptual problem by “rewiring” the whole library to a less duplicative design (e.g. a only a per-player cleanup list with no local copies kept in stream nodes).

So are there some good suggestions of how to do a minimal fix on the present design so that “double frees” are not so easily made?

2

Briefly, the problem is that Pchain copies the input event.

I think it makes more sense to continue this in the bug report https://github.com/supercollider/supercollider/issues/4806.

hjh

3

Ok as far as the bug goes, but I’d be more interested in a general design discussion here. Basically in the present design the idea seems to be that once a stream “decides” to produce nil, it executes its own accumulated cleanups. Since cleanups have been copied/sent downstream as well, the problem is how to make sure they don’t get re-executed downstream later on. The conundrum is that a stream that has just decided to output nil also has to send downstream info to the effect of having all cleanups that it has just executed descheduled. Unfortunately nil can’t have any side-info attached to it.

Lookig at the code for EventStreamCleanup.exit, I see it stuffs removeFromCleanup in the event. But if the event doesn’t get sent and nil gets sent instead downstream, how does that help?

4

I’m afraid I haven’t got the bandwidth for that at the moment, sorry.

What I was saying in the github issue is that it does work if the child (which is stopping/cleaning up) adds to removeFromCleanup in an event to which the parent (I’d call this “upstream” but never mind) has access. Unfortunately the right hand doesn’t know what the left is doing, and many pattern classes copy the event (without considering the impact on cleanup) so that they are modifying a new copy to which the parent does not have access.

I wouldn’t mind looking at code sketches that solve it, either by consistently not copying inevent or by passing back an enhanced “nil-with-side-data” from the stream that’s ending. But I don’t have energy right now to just talk about it at length, sorry.

hjh

5

I see you (hope to) rely on a shared (reference) to the same event to pass this info. I also see events are basically passed by reference in function calls, so that can indeed work if the callee (“sub-pattern”) writes into its “inevent” the arg the clean-up removal info…

f = { |x| x[\a] = 1 }
e = ()
f.(e)
e // ( 'a': 1 )

Since there’s no notion of cleanup for non-event streams, this would be good enough to solve all bugs of this kind if the implicit “call-by-ref” is not broken by any copies… I think the documentation of EventStreamCleanup.exit should stress this though.

6

The cleanup system can be quite convoluted in it’s implementation, and I’ve seen some variants of the inconsistent behavior other times as well. This area clearly needs some love. One thing that would be incredibly helpful to actually start making sense of these issues is if we had good unit tests pinning down the expected behavior. This would allow for better discussion of what the expected behavior is, and could represent some specific test cases that need to be fixed.

@RFluff, if you have the bandwidth, would you consider capturing some of the scenarios you’ve been posting about (related to clean-up) in some unit tests, and proposing a pull request? Failing tests are okay - we can comment them out until they are passing, but they’ll at least represent known bugs.

4 Likes
7

So this post – https://github.com/supercollider/supercollider/pull/4736#issuecomment-596803787 – got me thinking overnight.

Cleanup is about side effects

Pure functional streams don’t need cleanup because they don’t make any messes.

With EventStreamPlayer, the whole point is the side effect of playing the events. And, you want to be able to stop the side effects on demand (allowing for differences of opinion whether “stop” means only to stop new side effects, or to stop the currently-active side effects immediately). Cleanup needs to work in both directions: bottom-up (the stream stopped, and needs to clean up its own stuff) and top-down (the user ordered the whole thing to stop).

The above github comment seems to be proposing that a pure data stream, e.g. Pseries(0, 1, 10), should be just as “clean-uppable” as an event stream. Which raises two questions: 1/ What is the side effect to clean up? 2/ What is the concept of stopping a pure data stream?

1/ In theory, you could have a Pfset({ set some state }, Pseries(...), { clear the state }), so, OK. (It’s not pure anymore, but OK.)

2/ We “stop” a pure data stream passively, by simply not requesting any more values. You can call stop on any routine at any time and it will yield nil after that point, but in practice, we don’t bother.

#2 suggests that maybe it should be the stream itself handling the cleanup.

Data streams with side-effects

(
r = Routine {
	var synth;
	loop {
		synth = Synth(\default, [freq: exprand(200, 800)]);
		0.4.wait;
		synth.release;
		0.1.wait;
	}
}.play;
)

r.stop;

Common usage pattern here. We can think of this as a data stream (0.4, 0.1, 0.4, 0.1…) with side effects.

There’s an 80% chance of hitting stop between Synth() and release – leaving a hanging node. So that’s a second reason to consider that maybe the proper home of the cleanup is the stream itself.

What if you could write something like this?

(
r = Routine {
	loop {
		var synth;
		var cleanup = { synth.release };
		synth = Synth(\default, [freq: exprand(200, 800)]);
		thisThread.addCleanup(cleanup);
		0.4.wait;
		thisThread.removeCleanup(cleanup);
		synth.release;
		0.1.wait;
	}
}.play;
)

And imagine something like that throughout the pattern library.

Implementation

I’m afraid I don’t have time to build this myself at the moment, but some thoughts:

  • Don’t put this into Routine. There are other types of Streams (FuncStream, StreamCollect etc.). IMO a “clean-upabble Stream” should be a wrapper class.

  • If you wrap a Routine in some other class, thisThread will be the Routine rather than the object handling cleanup. So you would need some other mechanism to get the current stream-with-cleanup. I don’t know what that should be right now.

  • For nested streams (call next on A, and A calls next on B, and B has a cleanup), I believe propagation upward would work something like:

    + StreamWithCleanup {
	var stream;
	next { |inval, cleanupDelta|
		var childCleanupDelta = IdentityDictionary.new;
		var result = stream.next(inval, childCleanupDelta);
		... update my own cleanup according to delta...
		... add child cleanups into passed-in cleanupDelta...
		^result
	}
}

    … and returning the result to the caller would give any parent streams the opportunity to update, and so on. I’m not sure if that’s a good way to pass cleanup changes around, though.

Hope that helps. It may be difficult to implement but probably a design improvement. Comments (including “no, that’s a terrible idea”) welcome.

8

For now I filed one more rather trivial bug in Pfset and split the Pproto one that was similar to Pchain’s to a different report as the Pchain one is basically fixed already thank to James’ quick thinking.

9

Not really. That approach will fail if the child pattern copies the input event.

hjh

10

I’ll have to think about the rest of the stuff you said, but regarding

The current behavior is obviously

(Pfset(
  { "begin".postln },
  Prout({ 1.yield }),
  {"cleanup".postln}).play) // only does begin whereas...

(Pfset(
  { "begin".postln },
  Prout({ 1.asEvent.yield }),
  {"cleanup".postln}).play) // does cleanup...

Alas nil is a valid Event of sorts as in Event streams also end with nil, but currently

(Pfset(
  { "begin".postln },
  Prout({ }), // yields nil
  {"cleanup".postln}).play) // doesn't do cleanup...

So I filed the aforementioned “trivial” bug report against Pfset on this final corner case. There are semi-valid pattern idioms that basically produced that code-equivalent, e.g. if one sets ~cnt to 0 before calling the code bellow:

Pfset({ "begin".postln }, Pbind(\dur, Pseq([1], ~cnt)), {"cleanup".postln}).play
11

Of course I know that Pfset doesn’t currently work with non-events (which is why I said “in theory”).

The point is that it’s easy to conceive of converting a pure functional stream into a non-pure one (the pure one wouldn’t need cleanup but the non-pure one might). The precise mechanism to do that conversion doesn’t matter at all.

hjh

12

@scztt I’m trying to do this… I know it’s gonna sound like another rant, but UnitTest needs its test functions to be class methods… which needs a class library recompile for small/incremental changes to tests… which triggers annoying IDE bugs on Windows: the IDE doesn’t crash thankfully, but it often fills the Post doclet with “irrelevant” exceptions of its own…

And test methods don’t take any arguments, so I cannot hack around this by doing a “totally generic” test that just calls back into a function passed as argument… And yeah runTest does lookup into class.findMethod so I cannot just addUniqueMethod on existing TestUnit instance. For a reasonable workflow I basically have to do my own “testing framework” and then “backport” the tests into UnitTest. Another level of annoyance in SC…

13

There’s no requirement that unit tests be built in classes, that’s just a common pattern used in core library tests.

1 Like
14

Thanks, I saw the UnitTestScript feature, but for some reason it never works for me. I’ve put the script file next to some (custom) classes I can definitely load in SC but UnitTestScript.findTestScripts never manages to find anything… It might be an issue with it running on Windows, I don’t know. (I’ve made sure the file name ends in _unittest.scd, but still nothing.)

Thankfully it does work though when instantiated directly with a full/absolute path, as in your example. (Too bad the help file didn’t mentioned that method/issue…)

15

@jamshark70: Commenting here since github is down (for me anyway): your on-line Thunk fix, fixed almost everything my test suite tests. The only failure left was

UNIT TEST.............
There were failures:
an UnitTestScript: MyTest - Cleanup propagation { |x| Psetpre({ ~gg = 8; ~zz = 9; }, x) } <> { |x| Pchain(x) } inits: 1, cleanups: 0
an UnitTestScript: MyTest - Cleanup propagation { |x| Psetpre({ ~gg = 8; ~zz = 9; }, x) } <> { |x| Pchain(x, x) } inits: 2, cleanups: 0

I remember looking at Psetpre before and thinking it needed a separate fix… but I don’t recall what the issue was, alas.

But you’ve made PmonoArtic unkillable, as I suspected:

PmonoArtic(\default, \dur, 0.1).play // try stopping this now

For anyone else reading here, the Thunk fix is simply:

+ EventStreamCleanup {

	addFunction { |event, function |
		if(event.isKindOf(Dictionary)) {
			function = Thunk(function); // jhj
			functions.add(function);
			event[\addToCleanup] = event[\addToCleanup].add(function);
		};
	}
}

In case github doesn’t come up quickly again, the cleanup-test suite:

/* an UnitTestScript for cleanup chaining */
{ |test|
	var exhaust = { arg pat, ie = ();
		var cu = EventStreamCleanup.new, oe = nil, str = pat.asStream;
		var hist = List().add(cu.functions.size);
		while({
			oe = str.next(ie);
			cu.update(ie);
			hist.add(cu.functions.size);
			hist.size < 100 && oe.notNil});
		[hist, ie];
	};

	var t10y = {arg patfn;
		var icnt = 0, ccnt = 0;
		var inj = Pfset({icnt = icnt + 1}, p{|ev| ev.yield}, {/* "CUP!".postln;*/ ccnt = ccnt + 1});
		exhaust.(patfn.(inj));
		[icnt, ccnt];
	};

	var testPfs = [
		{ |x| Pn(x, 1) },
		{ |x| Pn(x, 3) },
		{ |x| Pn(x, 0) },
		{ |x| Pchain(x) },
		{ |x| Pchain(x, x) },
		{ |x| Pcollect({ |ev| ev }, x) },
		{ |x| Pcollect({ |ev| ev.copy }, x) },
		{ |x| Pseq([x]) },
		{ |x| Pseq([x, x]) },
		{ |x| Pseq([x, (y: 10)]) },
		{ |x| Psetpre({ ~gg = 8; ~zz = 9; }, x) },
		//{ |x| Ppar([x, x]) } throws
	];


    "Beging test".postln;

	testPfs.do { |pf|
		var cntPairs = t10y.(pf);
		test.assert(cntPairs[0] == cntPairs[1], "Cleanup propagation" +
			pf.cs + "inits:" + cntPairs[0] ++ ", cleanups:" + cntPairs[1]);
	};

	testPfs.do { |pf|
		testPfs.do { |pf2|
			var cntPairs = t10y.(pf <> pf2);
			test.assert(cntPairs[0] == cntPairs[1], "Cleanup propagation" +
				// (pf <> pf2).cs doesn't print anything useful
				pf.cs + "<>" + pf2.cs + "inits:" + cntPairs[0] ++ ", cleanups:" + cntPairs[1]);
		};
	};
}
16

If on top of that (Thunk) you actually use this Pchain (v4) variant, it fixes everything, without touching Psetpre:

Pchain : Pattern {

	var <>patterns;
	*new { arg ... patterns;
		^super.newCopyArgs(patterns);
	}
	<> { arg aPattern;
		var list;
		list = patterns.copy.add(aPattern);
		^this.class.new(*list)
	}


	embedInStream { arg inevent;
		var streams, cleanup = EventStreamCleanup.new;
		var localEvent;
		streams = patterns.collect(_.asStream);
		loop {
			localEvent = inevent; // v4: no copy allowed
			streams.reverseDo { |str|
				localEvent = str.next(localEvent); // v2
				["J4", localEvent, inevent, inevent.identityHash.asHexString].postln; //
				cleanup.functions.size.postln;
				//if(localEvent.isNil) { ^cleanup.exit(inevent) }; // v2
				//if(localEvent.isNil) { inval.putAll(inevent); ^cleanup.exit(inval) }; // v3
				if(localEvent.isNil) { ^cleanup.exit(inevent) }; // v3x
			};
			cleanup.update(localEvent);
			inevent = yield(localEvent);
		};
	}

	storeOn { arg stream;
			stream << "(";
			patterns.do { |item,i|  if(i != 0) { stream << " <> " }; stream <<< item; };
			stream << ")"
	}

}

UNIT TEST.............
There were no failures

But some of that zero-copy stuff is probably overkill now with the Thunk.

1 Like
17

Thanks for posting the test script. I’ll play with it later.

Psetpre({ ~gg = 8; ~zz = 9; }, x)

Psetpre’s signature is Psetpre(name, value, pattern) – I wouldn’t expect this to work.

On the phone now, I’ll try later.

hjh

1 Like
18

Funnily I had copied that from another unit test TestPatternProxy (l. 92). So I guess that needs fixing too to actually be more meaningful.

If you replace in my unit test that Psetpre with

		{ |x| Psetpre(\gg, 8, x) },

There are no failures with the Thunk addition alone (no need to change Pchain). I’m not sure what happens in Psetpre if the value stream (i.e. that 8) becomes nil first though, should probably test separately. Actually that seems fine too; I’ve added to the testP[attern]f[functions]s

		{ |x| Psetpre(\zz, Pseq([nil]), x) },
19

OK, I see. How about this?

Thunk : AbstractFunction {
	// a thunk is an unevaluated value.
	// it gets evaluated once and then always returns that value.
	// also known as a "promise" in Scheme.
	var function, value;

	*new { arg function;
		^super.newCopyArgs(function)
	}
	value { |... args| ^this.prEvaluate(\valueArray, args) }
	valueArray { |... args| ^this.prEvaluate(\valueArray, *args) }
	valueEnvir { |... args| ^this.prEvaluate(\valueArrayEnvir, args) }
	valueArrayEnvir { |... args| ^this.prEvaluate(\valueArrayEnvir, *args) }

	prEvaluate { |selector, args|
		^value ?? {
			value = function.performList(selector, args);
			function = nil;
			value
		}
	}
}

hjh

1 Like
20

There is actually a cleanup problem of sorts in Psetpre…

r = Psetpre(\bah, Pbind(), Pbind()).asStream
r.next(()) // -> ( 'bah': (  ) ) as expected

p = Pfset({ \INI.postln }, Pfunc {|ev| ev}, { \CLN.postln })
r = Psetpre(\bah, p, Pseq([nil])).asStream
r.next(())  // does cln!

r = Psetpre(\bah, p, Pseq([1])).asStream
[r.next(()), r.next(())]  // NO cln!

r = Psetpre(\bah, p, Pbind(\meh, Pseq([1]))).asStream
[r.next(()), r.next(())] // CLN again

r = Psetpre(\bah, p, Pseq([ () ])).asStream
[r.next(()), r.next(())]; // also DOES cln!

Amusingly Psetpre only works correctly with cleanup from the value stream only if the pattern stream is nil right away… if it’s not actually an event stream. This happens because Psetpre passes the addToCleanup messages through the pattern stream before calling update. Arguably it could call update twice, first on the event return from the value stream and then on event obtained from the pattern stream. But I guess the 2nd example in this chunk is conceptually buggy anyway, although it runs.

On the other hand, I’ve managed to break it with a somewhat more more legit example:

p = Pfset({ \INI.postln }, Pfunc {|ev| ev}, { \CLN.postln }) 
r = (Psetpre(\bah, Pchain(p), Pseq([nil]))).asStream
r.next(()) // only ini, no cleanup

This is like the 1st one above, not the 2nd, i.e. pattern stream returns nil (which could happen from an event stream), it’s just the value stream that gets an extra Pchain wrapper that causes the break. And something like the 3rd & 4th examples in the 1st block also break it with that extra wrapper.

r = (Psetpre(\bah, Pchain(p), Pseq([ () ]))).asStream
[r.next(()); r.next(())] // also no cleanup

r = Psetpre(\bah, Pchain(p), Pbind(\meh, Pseq([1]))).asStream
[r.next(()), r.next(())] // no cleanup either

So probably needs some attention… but I’ll look into fixing PmonoArtic first as it’s much bigger show stopper.