Kernel-mt.1566 interfering with testValueWithinTermination test

[isCzech]

Hi Marcel @marceltaeumel ,

I noticed that since merging Kernel-mt.1566 on May 18, 2024 the #testValueWithinTermination test stopped working. Not only that but my image freezes when running the test and not even CMD+dot can interrupt it, hence I have to kill it.

The image 23037 works fine (before merging Kernel-mt.1566) and in 23043 (with Kernel-mt.1566) the test fails ant the image freezes irreparably.

I haven't figured out yet WHY Kernel-mt.1566 causes this behavior and I'm really curious about it. I haven't followed the discussion about Kernel-mt.1566 but I'm hoping you might spot the problem right away :)

To add some context: The failing test was added by Christoph to test a fix (Chronology-Core-jar.91) of a bug in #valueWithin:onTimeout: in January 2024 (in case this might have anything to do with it).

Thanks,
Jaromir

[marceltaeumel]

We use update maps for critical changes such as changing requently used variables. So, you must first load Kernel-mt.1565. After that, you can load Kernel-mt.1566.

Best,
Marcel

[isCzech]

I may have confused you... in all trunk images since Squeak6.1alpha-23043-64bit/ the #testValueWithinTermination test crashes - not that I merged Kernel-mt.1566 in my image. I just tracked down what change in the Trunk caused the test start crashing and found it happened between 23037 and 23043 when Kernel-mt.1565 and Kernel-mt.1566 have been introduced. Well, I can't actually say it's because of those two, hmm, something else may have changed too. Or maybe I'm still missing something :)

[marceltaeumel]

This changeset reverts those changes from 1565/1566 to some extent.
revert-allprocesses-list.1.cs

After that, the #testValueWithinTermination works again. Interesting!

[marceltaeumel]

I suspect a side-effect of Process class >> #new that I was not aware of?

[marceltaeumel]

Yeah, that AllProcessesLock (mutex) does not work because there can be nested calls of Process new? Or some other condition where the critical section is not (yet) left? Or is this a new bug with Mutex >> critical: altogether?

new
	"Keep track of the processes being created and alive to avoid the need for #allSubInstances in tools such as the ProcessBrowser."

	| newProcess |
	newProcess := super new.
	AllProcessesLock critical: [AllProcesses add: newProcess].
	^ newProcess

[isCzech]

I guess I know what happened: Christoph's test was supposed to test a particular situation AFTER the watchdog process had been created. However, Marcel's Process class >> new made Christoph's runUntil: condition in the test (see below) stop INSIDE the critical section of the creation of the watchdog process in Process class >> new - i.e. completely misplacing the original purpose of the test. As a result the half-created watchdog process is now being tested for isSuspended by an assert and that's where the image crashes irreparably. (cf. if the assert is commented out the test passes, no crash)

The questions that come to mind are:

1. Why exactly the image CRASHES - is it an expected (inevitable) behavior or a bug somewhere?
2. The Process class >> new possibly creates additional suspension points (especially inside the critical section) where - if the process is terminated before it's even fully created - the system may crash.
3. Or MAYBE these are not real suspension points as perceived by the VM but only artificial ones created by the manual stepping - however this is beyond my knowledge to say which it is. And I'd love to be able to tell :)

testValueWithinTermination
	"Regression test for synchronization issue (https://github.com/squeak-smalltalk/squeak-object-memory/issues/108)"

	| process watchdog |
	process := [[100 milliSeconds wait] valueWithin: 200 milliSeconds onTimeout: []] newProcess.
	"simulate a possible context switch in primitiveResume"
	process runUntil: [:c | c isClosureContext and: [c size > 0 and: [(watchdog := c top) isKindOf: Process]]]. 
	self assert: watchdog isSuspended.                                                          "<---- this assert causes the crash -----"
	
	process step.
	process terminate.
	
	250 milliSeconds wait.
	self assert: watchdog isTerminated.
	self assert: process isTerminated.

Christoph @LinqLover - what do you think? Should the test work even under these changed conditions or should it just be amended to fulfill the original purpose? Thanks!

[eliotmiranda]

Re 3. "Or MAYBE these are not real suspension points as perceived by the VM but only artificial ones created by the manual stepping - however this is beyond my knowledge to say which it is. And I'd love to be able to tell :)" I am updating Process's class comment with a section on suspension points. We can co edit this until we reach consensus.

[isCzech]

I am updating Process's class comment with a section on suspension points. We can co edit this until we reach consensus.

With pleasure!

Why exactly the image CRASHES - is it an expected (inevitable) behavior or a bug somewhere?

My take is if Process class>>new gets interrupted/preempted inside the critical section, no other new process including system processes can be created which results in the image crash. Enclosing AllProcessesLock critical: [AllProcesses add: newProcess] in #valueUnpreemptively makes the test go through without crashing the system but would that ok as a solution?

Porcess class >> #new
	"Keep track of the processes being created and alive to avoid the need for #allSubInstances in tools such as the ProcessBrowser."

	| newProcess |
	newProcess := super new.
	[AllProcessesLock critical: [AllProcesses add: newProcess]] valueUnpreemptively.
	^ newProcess

[isCzech]

Oops, the previous post's code example should read:

Process class >> #new
	"Keep track of the processes being created and alive to avoid the need for #allSubInstances in tools such as the ProcessBrowser."

	| newProcess |
	newProcess := super new.
	[AllProcessesLock critical: [AllProcesses add: newProcess]] valueUnpreemptively.
	^ newProcess

Apologies for the noise.

[isCzech]

Hi @LinqLover , @marceltaeumel , @eliotmiranda

ok, the problem here is Christoph's test by accident discovered a situation where Process class >> #new is suspended inside the Mutex >> #critical section (at the first bytecode, so it's a real suspension point - thanks, Eliot!), hence preventing ANY new process from being created. The test #testValueWithinTermination then proceeds to an assert that fails and tries to open a debugger notification, i.e. a new process -> hence the system freezes and can't be unfrozen.

Now, what to do?

1. As I think the issue is real, not artificially created - even if the chances of happening it are probably slim - should it be addressed somehow? Like e.g. suggested above? Please advise, this is beyond my experience with the system.
2. Christoph's test should be amended to do it's original purpose to test the watchdog, not the Process class >> #new
3. Maybe add a test to test Process class >> #new to document/prevent the behavior observed here?

[nicolas-cellier-aka-nice]

1. My experience is that even if the chance is slim, it will happen. We must correct it.

If we protect with valueUnpreemptively, then we do not need the Mutex critical: inside the block.

2. Yes, the test should be fixed so as to test it's original purpose
3. I don't well see how to write a reproducible test, but thru simulation. For Mutex critical: I trust you that there are real suspension points, not artificial ones. But how would we simulate valueUnpreemptively? if the simulator can preempt, then it's purely artificial and the tests doesn't test anything. If the simulator doesn't preempt, then we don't test much either?

[nicolas-cellier-aka-nice]

OK, it's clear that the tested process (the local inst. var. in testValueWithinTermination) is suspended inside the critical section (while creating the watchdog process), which prevents further Process creation.

But I would expect that the ensure block would be executed upon process termination, and that the Mutex would then be disowned...

This does not happen because as you noticed, the assertion failure was caught above in runCase: and the test interrupted before executing process terminate.

Hence, the process is never terminated, the ensured block is never executed, and the Mutex never disowned.

This is this process that I see in the crah.dmp if I kill -USR1 the VM running the frozen image

Process      0x11fdcc3c8 priority 40
0x16b6f3830 M Mutex>critical: 0x11d43da00: a(n) Mutex
0x16b6f3870 M Process class>new 0x11ce96ee8: a(n) Process
0x16b6f38a8 M Process class>forContext:priority: 0x11ce96ee8: a(n) Process
0x16b6f38e8 M FullBlockClosure(BlockClosure)>newProcess 0x11b8f2f08: a(n) FullBlockClosure
0x16b6f3930 M TestValueWithinFix(TestCase)>timeout:after: 0x11b8f2008: a(n) TestValueWithinFix
0x16b6f3970 M [] in TestValueWithinFix(TestCase)>runCase 0x11b8f2008: a(n) TestValueWithinFix
...snip...

If I modify the test like this:

[self assert: watchdog isSuspended.
process step]
	ensure: [process terminate].

then I can run the TestValueWithinFix suite again.

Of course, I cannot interactively run TestValueWithinFix new testValueWithinTermination because it will spawn a new UI process for debugging, which will completely freeze the image again.

The more I think of it, the more I'm convinced that it's a precise case for which valueUnpreemptively has been invented.

[eliotmiranda]

The more I think of it, the more I'm convinced that it's a precise case for which valueUnpreemptively has been invented.

Agreed