Testsuite: Fix bugs in TestBuffer_Server

[jamshark70]

Purpose and Motivation

Couple of things:

1. It was pointed out elsewhere that aRoutine.stop.reset is not safe -- it does not prevent double scheduling. Instead, the timeout routines in this class should be disposable -- use once, and then the next timeout should create a new routine.

2. loadToFloatArray must be separately forked, if it's to be used with a condition-hang. (Why? loadToFloatArray uses forkIfNeeded, so it will run in the test's routine. But the test's routine is being caused by the condition, so loadToFloatArray never completes. test_cheby was hanging because of this.)

Types of changes

• Bug fix

To-do list

• Code is tested
• All tests are passing
• This PR is ready for review

[avdrd]

Regarding the first issue, the documentation for Routine should probably have a warning about this .stop.reset anti-pattern. I'm not saying that should be done as part of this commit. If one reads the help page for Routine very carefully, the pitfall is somewhat foreseeable, but clearly code that fell into it got past code review, so... it is not so obvious perhaps, thus worth making more explicit in the help.

[dyfer]

Thanks @jamshark70

Without analyzing the logic thoroughly... As I don't see these tests failing before, nor being excluded from the test suite, is the issue here that they were not catching the issues appropriately?

[jamshark70]

This PR is addressing two issues:

No. 1 -- .stop.reset -- it happens in these cases not to exhibit broken behavior, but it's really not a good idea.

One motivation for changing this is that, in a conversation (I forget whether it was on github or the forum), the use of .stop.reset within the test suite was held up as evidence that this usage is canonical and fully supported. It isn't, and shouldn't be:

(
r = fork {
	inf.do { |i|
		i.postln;
		1.0.wait;
	}
};
)

0.0
1.0
2.0
...

r.stop.reset;

0.0
1.0
...

You might have thought it was going to stop, and the reset would apply to the next time it played. In fact, .reset cancels the stopped status -- so the end result is that the thread continues, back from the beginning. The usage in this test class does not suggest that this is what the author wanted.

However... when I submitted the PR, I was trying to make minimal changes. Now I think I should do it again, and replace all of these ad-hoc timeout threads with CondVar's timeout feature.

No. 2 -- loadToFloatArray's sync behavior is not the same as other asynchronous operations, when it's called within a thread. This is because of forkIfNeeded.

UnitTest.runTest("TestBuffer_Server:test_cheby");

Note that every one of the subtests takes a full second. Now, there's no way that generating a 512-sample buffer and retrieving it in the client should take that long. Each of these should be finishing within a few dozen milliseconds (and in fact, that's what happens with the fix here). Instead, the only reason why the test advances is because of the timeout. If we are relying on timeout logic to advance through a successful test, this is formally incorrect.

Note the difference:

// typical async op -- getn
(
s.waitForBoot {
	var cond = CondVar.new;
	var buf = Buffer.alloc(s, 16384, 1, { |buf| buf.sine1Msg([1]) });
	
	s.sync;
	
	"calling getn".debug;
	buf.getn(0, 10, action: { |data|
		"getn action".debug;
		data.postln;
		cond.signalOne;
	});
	"pausing thread".debug;
	cond.wait;
	"done".postln;
	buf.free;
};
)

// loadToFloatArray with the same condition-signaling logic
(
s.waitForBoot {
	var cond = CondVar.new;
	var buf = Buffer.alloc(s, 32, 1, { |buf| buf.sine1Msg([1]) });
	
	s.sync;
	
	"calling loadToFloatArray".debug;
	buf.loadToFloatArray(action: { |data|
		"loadToFloatArray action".debug;
		data[0..10].postln;
		cond.signalOne;
	});
	"pausing thread".debug;
	cond.wait;
	"done".postln;
	buf.free;
};
)

| getn output      | loadTo... output           |
|------------------+----------------------------|
| calling getn     | calling loadToFloatArray   |
| pausing thread   | loadToFloatArray action    |
| getn action      | FloatArray[ ... data ... ] |
| [ ... data ... ] | pausing thread             |
| done             |                            |

Note that "done" never happens in the second case!

This is what's happening in test_cheby, and other places where loadToFloatArray is being used. The test isn't finishing in the right way. You don't see a failure because the timeout logic kicks in and covers over the confusion over threading behavior -- but the absence of a failure does not mean that the test is optimally written.

[dyfer]

Thanks you for extensive answer, it all makes sense.

Now I think I should do it again, and replace all of these ad-hoc timeout threads with CondVar's timeout feature.

Are you planning to update this PR or create a new one after this one is merged?

[jamshark70]

Are you planning to update this PR or create a new one after this one is merged?

Just updated 😁 -- btw, it's kind of remarkable how much more lucid the timeout logic is, when using CondVar throughout. You just waitFor(3) and make sure the action function signals, that's it.

[dyfer]

Thanks! Looks good to me.

Just to make sure - anything left here before I merge @jamshark70 ?

[dyfer]

All right, merging