Process Changes Meta Pull Request

[guillep]

It's been really difficult to review the batch of issues (#8567 , #8845, #9137, #8993, #8509, #10383).
We have spent in the last internal sprints several hours taking a look at them, but it is a reality that those changes are difficult to review, some like #8567 are very big, their effects in the system are difficult to foresee independently and even worse in combination.

This PR rebases and joins together all changes in question with the objective of testing the combination of all of them.
We then propose that we eagerly merge this (meta) PR and be ready to quickly apply hot-fixes as soon as possible or revert it in case of fire.

[isCzech]

Hi all,

Thanks for looking at the PRs, here are my comments:

#9137 is an isolated adjustment of an inconsistency in exceptions handling I expect will not interfere with anything here.

#8509: I suggest you add #10383 into the mix as it addresses (and fixes) the root cause of the "stepOver bug" (#8443), and I think #10383 supersedes the fix in #8509 (the test in #8509 should of course be valid for #10383 as well).

#8993: this is a part of a larger project (of mine) rewriting #terminate. I extracted this part for Pharo to work without clashes with other code; however #8567 was so big I couldn't foresee all possible interactions - so sorry if it does. The main objective of this PR was to eliminate the terrible "unwind error" and extend a bit the scope of unwinding during termination (complete ALL half-way through unwind contexts).

#8845 was an attempt to fix a bug in #isTerminated and avoid hardcoding exact conditions like "pc + 1 = self endPC". This PR will definitely interfere with #8567; I don't exactly know what the "grand plan" to make this super-PR work is but I consider this #8845 of less importance and could be withdrawn if it meant substantial adjustments on #8567, and an alternative solution merged later.

Summary:
If I might suggest (but I'm fully aware your experience exceeds mine by orders of magnitude) I'd get rid of the "easy" non-disruptive stuff first - i.e. include #10383 to get rid of the "stepOver bug" (or even merge it separately first; the impact of the change is very isolated - but the consequences of the fix quite substantial).

Best regards,
Jaromir

[guillep]

Thanks for the quick reply, I'll check #10383 and put it together here

#8993: this is a part of a larger project (of mine) rewriting #terminate. I extracted this part for Pharo to work without clashes with other code; however #8567 was so big I couldn't foresee all possible interactions - so sorry if it does. The main objective of this PR was to eliminate the terrible "unwind error" and extend a bit the scope of unwinding during termination (complete ALL half-way through unwind contexts).

I'm super sad about the current state of process termination. I spent over lunch doing a sketch of a different solution: The idea being that no process can terminate another process. A process can only terminate itself. This would imply for example:

• processes should be able to join others

Process >> join

	"Wait the calling process until the receiver process terminates.
	If the process is already terminated, return directly.
	Otherwise, wait in a Semaphore"
	self isTerminated ifTrue: [ ^ self ].

	(joinSemaphore ifNil: [ joinSemaphore := Semaphore new ]) wait

• termination is eventual: when the terminated process runs it will terminate. This would mean that semantics are weaker because now terminate does not force a termination but requests a termination. The process will terminate when it cans (the catch is that it will only do it if it can). A pro: the code is much simpler and we can throw away a lot of complex code.

Process >> terminate2
	
	"Request termination of the receiver process.
	
	The current process will eventually terminate itself"
	suspendedContext := [
		self doTerminationFromYourself.
		joinSemaphore signalAll.
	] asContextWithSender: suspendedContext

• Although the above does not work with the changes in this PR, and needs to be defined as something like:

Process >> endProcess
	"When I reach this method, I'm terminated. Suspending or terminating me is harmless."

	<isTerminated>
	thisContext terminateTo: nil.   "set thisContext sender to nil"
-->	joinSemaphore ifNotNil: [ :s | s signalAll ].
	self suspend.

• Another detail we were looking at is how process termination interacts with Semaphores, which we find rather fragile because ensure blocks need to know the method structure of semaphores to work properly. The problem being that when executing the ensure block, we really don't know if the critical section was really taken or not. An idea would be to use the result of self wait as a token of the taken critical section. Something as follows. The idea is that with that change, the ensure block should be safe to execute regardless of when/how stack unwind happens.

Semaphore >> critical: mutuallyExcludedBlock			
	| blockValue caught |
	caught := false.
	[
		caught := true.
		self wait.
		blockValue := mutuallyExcludedBlock value
	] ensure: [caught ifNotNil: [self signal]].
	^blockValue
===>
Semaphore >> critical: mutuallyExcludedBlock			
	| blockValue caught |
	[
		caught := self wait.
		blockValue := mutuallyExcludedBlock value
	] ensure: [caught ifNotNil: [self signal]].
	^blockValue

Another thing to take into account is that many languages don't support forcing the termination of processes

• .NET Thread.abort()is not supported anymore https://docs.microsoft.com/en-us/dotnet/api/system.threading.thread.abort?view=net-6.0
• Java Thread.stop(), is deprecated, marked to remove https://download.java.net/java/early_access/loom/docs/api/java.base/java/lang/Thread.html#stop()

Maybe a large discussion should be done on this topic and proposed as a phep (https://github.com/pharo-project/pheps)

[dionisiydk]

Hm, It can be tricky to manage such composite PR.
CI shows strange test failures in bootstrap image while they pass on full image (see SUnitTest's)
The analysis will require disabling some of the changes here. But we (original authors) have no rights to modify this PR.

If we would at least know the CI status of each separate PR that would help. But the main changes were not mergeable for a long time. Do we know the CI status for #8993 ? I can't find it on Jenkins

[dionisiydk]

Maybe a large discussion should be done on this topic and proposed as a phep (https://github.com/pharo-project/pheps)

Why not use pharo-dev ML for this?

[dionisiydk]

• termination is eventual: when the terminated process runs it will terminate. This would mean that semantics are weaker because now terminate does not force a termination but requests a termination. The process will terminate when it cans (the catch is that it will only do it if it can). A pro: the code is much simpler and we can throw away a lot of complex code.

Process >> terminate2
	
	"Request termination of the receiver process.
	
	The current process will eventually terminate itself"
	suspendedContext := [
		self doTerminationFromYourself.
		joinSemaphore signalAll.
	] asContextWithSender: suspendedContext

It sounds very elegant to me. But probably there are a lot of underwater stones there.

[dionisiydk]

Another thing to take into account is that many languages don't support forcing the termination of processes

• .NET Thread.abort()is not supported anymore https://docs.microsoft.com/en-us/dotnet/api/system.threading.thread.abort?view=net-6.0
• Java Thread.stop(), is deprecated, marked to remove https://download.java.net/java/early_access/loom/docs/api/java.base/java/lang/Thread.html#stop()

I was always amazed by this Java "feature" . And diving over the internet never shown me any proper explanation.
On the OS level you are able to kill processes and nobody would ever tell you that it is a bad thing. But for the thread model in these nice languages it is forbidden or unimplemented. It's no sense to me.

Thread.Abort Method (System.Threading)

Raises a ThreadAbortException in the thread on which it is invoked, to begin the process of terminating the thread. Calling this method usually terminates the thread.

Error Page 404

[Ducasse]

Denis we can use both the mailing list and a pheps
the pheps should be able to summarize the discussions.
Because discussions can be long to follow and remember.

[dionisiydk]

• Another detail we were looking at is how process termination interacts with Semaphores, which we find rather fragile because ensure blocks need to know the method structure of semaphores to work properly. The problem being that when executing the ensure block, we really don't know if the critical section was really taken or not. An idea would be to use the result of self wait as a token of the taken critical section. Something as follows. The idea is that with that change, the ensure block should be safe to execute regardless of when/how stack unwind happens.

Semaphore >> critical: mutuallyExcludedBlock			
	| blockValue caught |
	caught := false.
	[
		caught := true.
		self wait.
		blockValue := mutuallyExcludedBlock value
	] ensure: [caught ifNotNil: [self signal]].
	^blockValue
===>
Semaphore >> critical: mutuallyExcludedBlock			
	| blockValue caught |
	[
		caught := self wait.
		blockValue := mutuallyExcludedBlock value
	] ensure: [caught ifNotNil: [self signal]].
	^blockValue

It's not that simple to just change the #wait primitive to return a flag. See the example:

• the processes A and B wait on critical section on semaphore - they are suspended and added to the semaphore waiting list.
• the active process C with higher priority finishes the critical section and signals the semaphore. It resumes the process A (first in the semaphore list).
• process C continues execution due to the hight priority and it decided to terminate the process A
• process A still did not continue execution (#wait did not return) and therefore the #caught variable is still nil
• process A terminates without #signal in #ensure section
• state fo semaphore become inconsistent - process B will never wake up.

We had long story about critical sections with @bencoman. And the final approach was proposed here: https://pharo.fogbugz.com/f/cases/19186. It requires new lock primitives with new semantics.

Generally speaking we should deprecate Semaphore>>critical: and always use Mutex instead. But it requires proper implementation based on locks.

[guillep]

• process C continues execution due to the hight priority and it decided to terminate the process A

Well, here with my model this does not hold. Process C will never force termination on process A.
So process A can resume, take its critical section, terminate itself, free the critical section, and die.

[guillep]

We are investigating the tests failing in the CI. Those tests do not fail locally.
I've seen there is a potential problem here:

assertTerminatedFailedChildProcessesFor: aTestCase

	| failedProcesses |
	failedProcesses := aTestCase failedChildProcesses.
	self assert: failedProcesses notEmpty.
	self assert: (failedProcesses allSatisfy: #isTerminated) 

1. First, having many assertions in a single test makes it difficult to follow. We are not sure what assertion is failing in the CI.
2. The implementation of failedChildProcesses is fragile and inefficient:

SUnitTest >> failedChildProcesses
	^Process allInstances 
		select: [: each | each name = ('failed child for ', testSelector)]

Moreover, this really depends on the GC. If the GC is called just before the processes are collected then the test fails!

If changing the assertion method by:

SUnitTest >> assertTerminatedFailedChildProcessesFor: aTestCase

	| failedProcesses |
-->   Smalltalk garbageCollect.
	failedProcesses := aTestCase failedChildProcesses.
	self assert: failedProcesses notEmpty.
	self assert: (failedProcesses allSatisfy: #isTerminated) 

I'm able to reproduce 4 of the CI failing tests locally.

[dionisiydk]

I'm able to reproduce 4 of the CI failing tests locally.

Right. SUnitTest's for process monitoring are kind of integration tests. Forked processes from related methods can be accumulated into an explicit instance variable. For example:

failedChildProcessTest
	"During this test forked process should signal error.
	It means that after fork we should give the process control"
	| process |
	process := ([ self error: 'error from child process'] newProcessNamed: 'failed child for ', testSelector.
	forkedProcesses add: process.
	process resume.
	Processor yield.

Then assertion will check these local processes. And it will be stable.

[dionisiydk]

SindarinDebuggerTest requires changes in Sindarin project: pharo-spec/ScriptableDebugger#14

[dionisiydk]

Our PRs #8567 and #8845 were not compatible to each other. And I think it leads to some broken tests here. I would exclude #8845 from this PR for now.
But the correct merge would be:

• use #isTerminated from my PR [Very critical fix] Processor>>#terminateRealActive #8567.
• modify #isEndOfProcessTermination to use Jaromir logic based on pragma (hasPragma here)

The reason why I suggest to postpone #8845 is because in my PR I followed the idea that the full stepping over the process should be equivalent to the normal execution which always ends up at the final #suspend message. Therefore the real termination status requires explicit check for the #pc to detect such condition. See #isEndOfProcessTermination method.
Thus a simple test proposed in #8845 is not enough. Stepping the process would end up at the start of the method where pragma is detected. And it will not be equivalent to the normal execution of a process: stepping will not execute "thisContext terminateTo: nil" part of #endProcess method.

[isCzech]

But the correct merge would be:

• use #isTerminated from my PR [Very critical fix] Processor>>#terminateRealActive [Very critical fix] Processor>>#terminateRealActive #8567.
• modify #isEndOfProcessTermination to use Jaromir logic based on pragma (hasPragma here)

Yes, that's what I'd suggest as well; #8845 is not that important - it doesn't fix bugs; just simplifies the logic.

[dionisiydk]

• process C continues execution due to the hight priority and it decided to terminate the process A

Well, here with my model this does not hold. Process C will never force termination on process A. So process A can resume, take its critical section, terminate itself, free the critical section, and die.

Do you mean this model?

Process >> terminate2
	"Request termination of the receiver process.	
	The current process will eventually terminate itself"
	suspendedContext := [
		self doTerminationFromYourself.
		joinSemaphore signalAll.
	] asContextWithSender: suspendedContext

It will not solve the problem. Process A will resume and perform doTermination logic which will not set the caught flag in the #critical: method and the ensure will incorrectly skip the semaphore signal.

[dionisiydk]

See my previous comments. #isDead already incorporates this logic but it needs to be adjusted for #endProcess method