[Cuis-dev] Unwind mechanism during termination is broken and inconsistent

Sat Jun 5 06:04:31 PDT 2021

Hi Juan,

> Hi Jaromir,
>
> I've just pushed your latest to GitHub. I included a note with links to this email thread in #terminate. I also added the new tests; and your examples as a class method in Process, for future reference.

You made my day, thanks :) It's been a great learning experience; I'd like to thank you for your valuable comments and feedback that helped me to stay on track. Also, I appreciate you keep Cuis' kernel simple and compact.

I'd like to share some remarks:

1) you might ask why #complete:to: is defined in Process and not in ContextPart? It should, really, but I wanted to keep it the same as in Squeak for the moment :)

2) there's currently a discussion (Christoph) what semantics is the most appropriate for closing the debugger: If you encounter a fatal error or the debugged process is damaged too badly, using #terminate might be "too strong" and lead e.g. to an infinite recursion of debuggers etc. For such cases a "light" version od #terminate (#terminateLight) without completing the halfway through unwind contexts would probably be more appropriate. For the most severe cases even a Kill option without any unwinds might come in handy. I imagine e.g. using #terminate for the standard closing of the debugger's window and the light version of #terminate for an explicit Abandon button.

3) I'd like to raise a question about non-local return's semantics during termination:

              **What takes precedence: a termination or a non-local return?**

Squeak has a method `valueUninterruptably  ^ self ifCurtailed: [^ self]` which in its comment says: "Even attempts to terminate (unwind) this process will be halted and the process will resume here.  A terminate message is needed for every one of these in the sender chain to get the entire process unwound."

It means Squeak authors originally intended to allow a process to escape its termination via a non-local return inside an unwind block but the implementation never worked.

The alternative behavior is: termination takes precedence and doesn't allow a non-local return to route the computation outside the unwind block. This is the behavior of the current #terminate.

I'd like to know your opinion about this matter; the new #terminate can be amended easily to allow for non-local returns take precedence over termination but I'm not sure it's really desirable - what do you think?

Example:
```
p := [
              [:exit | [Processor activeProcess suspend] "<-- terminate here"
                           ensure: [exit value]] valueWithExit.
              Transcript show: 'escaped termination'
] newProcess resume.
Processor yield.
[p terminate] fork
```
Current implementation will not print anything and just terminate the process p because there are no unwind blocks except `[exit value]`.

Originally, however, the non-local return inside `[exit value]` was intended for the process p to escape termination and continue executing, i.e. print 'escaped termination' and go on.

4) One last comment FYI: #unwindTo: method under ContextPart in my opinion suffers with the same bug as #terminate used to, and could be fixed similarly (there are currently no senders however). Check this example:

```
p := [
                           [:exit | [Processor activeProcess suspend] "<-- unwind here"
                                         ensure: [exit value]] valueWithExit
              ] newProcess resume.
Processor yield.
p suspendedContext unwindTo: nil
```

It incorrectly produces the BlockCannotReturn error. If you do `p terminate` instead of `p suspendedContext unwindTo: nil`, it works as expected.

Thanks again.

Best regards,

Jaromir

From: Juan Vuletich<mailto:juan at jvuletich.org>
Sent: Friday, June 4, 2021 15:44
To: Discussion of Cuis Smalltalk<mailto:cuis-dev at lists.cuis.st>
Cc: Jaromir Matas<mailto:mail at jaromir.net>
Subject: Re: [Cuis-dev] Unwind mechanism during termination is broken and inconsistent

Hi Jaromir,

I've just pushed your latest to GitHub. I included a note with links to this email thread in #terminate. I also added the new tests; and your examples as a class method in Process, for future reference.

This is an important piece of work. Thanks a lot!

Cheers,

--

Juan Vuletich

www.cuis-smalltalk.org<http://www.cuis-smalltalk.org>

https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev

https://github.com/jvuletich

https://www.linkedin.com/in/juan-vuletich-75611b3

@JuanVuletich

On 6/1/2021 9:09 AM, Jaromir Matas via Cuis-dev wrote:
Hi Juan,

I’m enclosing a finalized version of #terminate with the following changes:

1) whole termination code wrapped in #valueEnsured as suggested in the previous message

2) changed approach to BlockCannotReturn: error and adopted Christoph Thiede’s solution allowing the debugger to Proceed from the error to the sender of the failing non-local return. It makes sense in the debugger to leave all choices open for the user – Proceeding means ignoring the failed non-local return but why not - it could have been a typo after all :) So I’m taking back my suggestion to disable the Proceed button during unwinding – too restrictive. This change allows for debugging examples like:
[^2] fork
Or even:
[[self error] ensure: [^2]] fork
Previously these would crash the image.
To make this work with #terminate and notify the user I added a ProceedBlockCannotReturn warning.

3) #complete:to: is a helper method to improve readability of #terminte and also to deal with two special exceptions: BlockCannotReturn and MessageNotUnderstood. The latter exception was purposely written to allow for writing methods while debugging which introduced a potential for an infinite recursion I had to deal with (see notes in #complete:to: )

4) I’ve also integrated a fix to a stepOver bug in this example:
[^2] ensure: [Transcript show: 'done']
If you step through and then step over ^2 you get a  BlockCannotReturn error – that’s a bug
The problem is #return:from: supplies the first unwind context to #aboutToReturn:through: but this first unwind context was determined before the debugger inserted an #ensure context during the stepover simulation. My fix is to supply nil and let #resume:through: find the first unwind context at the right moment.
This fix modifies #resume:through: and #return:from: methods only and  is independent of the #teminate suite.

Please let me know if you find any inconsistencies… I hope not ;)

My best regards,
Jaromir

From: Jaromir Matas via Cuis-dev<mailto:cuis-dev at lists.cuis.st>
Sent: Tuesday, May 25, 2021 9:32
To: Juan Vuletich<mailto:juan at jvuletich.org>; cuis-dev at lists.cuis.st<mailto:cuis-dev at lists.cuis.st>
Cc: Jaromir Matas<mailto:mail at jaromir.net>
Subject: Re: [Cuis-dev] Unwind mechanism during termination is broken and inconsistent

Hi Juan,

I’ve explored a scenario when you are terminating a process, i.e. unwinding it’s ensure etc contexts and in the middle of it the process running the unwinds gets interrupted and terminated. In such case this process unwinds its stack and terminates but the original process is just left hanging as a suspended process (potentially indefinitely) or as a chain of contexts that get garbage collected eventually.

With the new terminate there’s an elegant solution to this scenario: wrap the whole termination code in #valueEnsured and the intruding process will correctly unwind both the original and the terminator process :) I’ve enclosed a test and modified #terminate only. Please take a look, I can’t see anything wrong with this approach.

Best regards,
Jaromir

From: Jaromir Matas<mailto:mail at jaromir.net>
Sent: Sunday, May 23, 2021 19:10
To: Juan Vuletich<mailto:juan at jvuletich.org>; cuis-dev at lists.cuis.st<mailto:cuis-dev at lists.cuis.st>
Subject: FW: [Cuis-dev] Unwind mechanism during termination is broken and inconsistent

Hi Juan,

I'm enclosing a test covering a situation where the top context itself is an unwind context. The test explores three scenarios based on the position where the execution was suspended: before entering the unwind block, right after setting complete := true but before executing the unwind block and after returning from the unwind block. #terminate should execute the unwind block in the first two cases but not in the last one.

Squeak has a convenient method Process>>#forBlock:runUntil: to step right into the desired position. So I just inlined its code into the test to make it work the same way :)

Regarding the tests demonstrating the unwind behavior in case of unhandled errors and subsequent Abandoning or Proceeding the debugger - it looks like Squeak has a machinery to do that (debugger tests) but I can't see anything similar in Cuis that would simulate opening and abandoning a debugger... Any advice?

Many thanks for all your feedback,

Best regards,

Jaromir

PS: Yesterday my message bounced back due to it’s size, I’m forwarding it again and trimming history.

From: Jaromir Matas<mailto:mail at jaromir.net>
Sent: Saturday, May 22, 2021 23:16
To: Juan Vuletich<mailto:juan at jvuletich.org>; Discussion of Cuis Smalltalk<mailto:cuis-dev at lists.cuis.st>
Subject: RE: [Cuis-dev] Unwind mechanism during termination is broken and inconsistent

Hi Juan,

Many thanks for your feedback.

I've made some final changes to the changeset:

1) extracted a repeating code to a method called Process>>complete:to: which calls #runUnwindUntilErrorOrReturnFrom: and deals with potential infinite loops caused by some errors.

2) as a result the unwind code pattern used in #terminate is almost identical to the one in #resume:through: - which I think is better for readability - when people can recognize a pattern right away :)

3) adjusted the part searching for the outer-most unwind context to catch even the top context if it's an unwind context.

4) In order to rein in the infinite loops or crashes caused by the BlockCannotReturn error I added a local variable isRecursive to BlockCannotReturn exception to be able to deal with a starting infinite loop. This is an example brought to my attention Christoph Thiede:

```
"Both statements need to be executed separately in a Workspace"
a := [true ifTrue: [^ 1] yourself]
[a value] on: BlockCannotReturn do: [:ex | ex resume]
```

With the proposed fix there's no need to change the isResumable setting of the BlockCannotReturn I suggested previously. Resuming doesn't hurt any longer :)

Regarding my suggestion to disable the Proceed button when debugging an error that happened during unwind - I'm no longer so sure it's a good idea to limit the freedom of the user in such a border situation like debugging an unwind error :) I suggest to leave it open, "use at your own risk" type of thing, for the moment.

> I think you are right in the previous email, and we'd integrate your latest change.I have already checked that none of the tests we recently added breaks because of it. If you could turn some of the arguments you provide into tests, it would be great.

I must admit I still haven't learned to build tests involving errors etc. :) But at least I updated and enclosed the list of examples that can be used as test scenarios and added some more extreme ones.

Juan, please review the enclosed changeset and let me know if you find something suspicious. Please feel free to change any names I picked or do any changes you find useful.

Best regards,

Jaromir

From: Juan Vuletich<mailto:juan at jvuletich.org>
Sent: Friday, May 21, 2021 21:08
To: Discussion of Cuis Smalltalk<mailto:cuis-dev at lists.cuis.st>
Cc: Jaromir Matas<mailto:mail at jaromir.net>; Hernan Wilkinson<mailto:hernan.wilkinson at 10pines.com>
Subject: Re: [Cuis-dev] Unwind mechanism during termination is broken and inconsistent

Hi Jaromir,

On 5/16/2021 5:26 PM, Jaromir Matas via Cuis-dev wrote:
Hi Juan,

I'm enclosing an updated changeset (please disregard the previous one); I made a stupid assumption about #runUntilErrorOrReturnFrom:. The new changeset leaves it untouched and adds its replica called #runUnwindUntilErrorOrReturnFrom: with the required functionality.

I think you are right in the previous email, and we'd integrate your latest change.I have already checked that none of the tests we recently added breaks because of it. If you could turn some of the arguments you provide into tests, it would be great.
I'd like to point out a little issue with the proposed semantics of nested errors during termination:

Below’s a simple example. If you abandon the first debugger it means you want to terminate the execution and unwind the terminated process but an error happens during the unwind. You get another debugger reporting the error and asking you what to do next - to debug the error or proceed. But proceeding a termination really means to proceed with the *unwind business only* and not to proceed with the original computation. Here's my point: Hitting Proceed will resume the original computation beyond the unwind blocks but Abandon will only finish the unwind procedure which is exactly what we want. In Squeak I simply disabled the Proceed button but in Cuis I couldn't see such a simple solution so I left it unresolved.

[self error: 'x1'] ensure: [
    [self error: 'x2'] ensure: [
        Transcript show: '2'].
    Transcript show: '1'].
Transcript show: '0'

---> you want to see '21' but proceeding the debugger will give you '210' where '0' is printed outside of unwind blocks.

So just that you know there's still a little inconsistency in the termination semantics.

I see. The issue is how to disable a button so it doesn't act? The simplest way is by saying `aButton lock: true`. We could add #enable and #disable to PluggableButtonMorph (calling self lock: ). Or perhaps call it #isEnabled: like in MenuItemMorph, although it is an ugly name. Can you prepare a change set like this? That would be great. I mean, I don't know where those calls should be made!
One more question: do you have an idea why BlockCannotReturn error is considered resumable? I'd say it's unresumable by definition…? Thanks :)

Well, that method is pretty old, and in the mean time, exception handling has gotten a lot better, thanks to several batches of improvements, latter ones by you. So, please provide a patch to change that, so it properly carries your initials.

best regards,

Jaromir

Thanks again Jaromir for this amazing work.

Cheers,

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20210605/42a1cd99/attachment-0001.htm>