Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
* A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
---
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier" + terminationMethod := #terminateAggressively. + self close + ! - self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true. + contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process"
+ terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil. interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil. Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz ==================== Summary ==================== Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160 Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092. The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode. Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail). Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107 Please review as I'm no debugger expert and this is just my suggestion. Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox) =============== Diff against Tools-ct.1160 =============== Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'! !Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context. Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery. * A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes. If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world. --- In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.! Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier" + terminationMethod := #terminateAggressively. + self close + ! - self close.! Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize super initialize. Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess]. externalInterrupt := false. selectingPC := true. + contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.! Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process" + terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.! Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher." contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil. interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil. Smalltalk installLowSpaceWatcher. "restart low space handler"!
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz ==================== Summary ==================== Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160 Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092. The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode. Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail). Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107 Please review as I'm no debugger expert and this is just my suggestion. Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox) =============== Diff against Tools-ct.1160 =============== Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'! !Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context. Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery. * A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes. If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world. --- In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.! Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier" + terminationMethod := #terminateAggressively. + self close + ! - self close.! Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize super initialize. Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess]. externalInterrupt := false. selectingPC := true. + contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.! Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process" + terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.! Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher." contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil. interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil. Smalltalk installLowSpaceWatcher. "restart low space handler"!
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz ==================== Summary ==================== Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160 Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092. The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode. Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail). Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107 Please review as I'm no debugger expert and this is just my suggestion. Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox) =============== Diff against Tools-ct.1160 =============== Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'! !Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context. Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery. * A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes. If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world. --- In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.! Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier" + terminationMethod := #terminateAggressively. + self close + ! - self close.! Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize super initialize. Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess]. externalInterrupt := false. selectingPC := true. + contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.! Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process" + terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.! Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher." contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil. interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil. Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
* [self halt] ensure: [1 second wait]. "Let the user know that something is happening" * [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" * [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." * [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
________________________________ Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel
Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel
Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel
Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de:
Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
* A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
---
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier"
+ terminationMethod := #terminateAggressively. + self close + ! - self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
+ contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process"
+ terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Christoph, Marcel, all,
let me address a few of Christoph's objections:
I would consider the #fork a workaround for an issue in #terminate
Not the case. It is a workaround but to get around something else; the thing is the debugger runs in the UI and when it waits until the termination is finished then from the user perspective the UI is frozen. Indeed this only affects closing the debugger via 'terminate process' debugger termination mode. I wonder, theoretically of course, would running the debugger in a separate process rather than in the UI be an option?
User interrupt will still work if something goes wrong with the termination, but at least the user will notice
I think the UI should stay responsive for termination invoked from the Debugger, at least for "harmless" errors (as opposed to “disastrous”); the difference between #terminate and #terminateAggressively is that in #terminate the process runs its own unwinds while in #terminateAggressively the unwinds are executed indirectly via simulation. Hence Abandon never loses control: the UI (the debugger) runs the unwinds and stays responsive, as opposed to #terminate where the UI hands full control back to the terminating process to execute its unwinds itself and thus the UI gets blocked when another error occurs (i.e. when the unwinding stops and waits for the default action to execute) :)
the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests
Nothing to do with that; the issue here is tests 21-24 simply have to be fixed, my bad :) (Marcel, apologies, please let’s remove tests 21-24 before I fix them)
we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised
Yes, but this is an issue with #terminateAggressively regardless of the forked termintion or not:
If you run this in the Workspace: p := [ [Semaphore new wait] ensure: [1/0] ] fork And then terminate by hand in the Workspace: [p terminate] fork
Then Abandoning the debugger via #terminateAggressively always generates the MNU you mentioned.
So are we talking about something like this? [self halt] ensure: [self error].
Right, terminate using 'terminate process' from halt, unwind and abandon - this should stay responsive all the way imo; freezing the UI is a symptom of something: #terminateAggressively deals with repeated errors itself (it's simulation and it unwinds indirectly) while #terminate simply lets a process unwind directly by itself - without any assumptions about e.g. debugger implementation; when an error occurs the corresponding handler is executed or a debugger is requested - and if the implementation doesn't employ two debuggers or doesn't spawn new UI then we're stuck :).
Anyway, how about to use the indirect termination in #windowIsClosing for 'terminate process' mode only? The mode is hidden well enough so it’s basically for my convenience :D
Thanks, Jaromir
--
Jaromír Matas
mail@jaromir.net
From: Thiede, Christophmailto:Christoph.Thiede@student.hpi.uni-potsdam.de Sent: Monday, June 13, 2022 21:06 To: squeak-devmailto:squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
* [self halt] ensure: [1 second wait]. "Let the user know that something is happening" * [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" * [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." * [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel
Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel
Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel
Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
* A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
---
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier"
+ terminationMethod := #terminateAggressively. + self close + ! - self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
+ contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process"
+ terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Christoph --
In your example, one would just have to #yield the UI process for the moment within #windowIsClosing. ;-)
I will revert that for now.
Best, Marcel Am 13.06.2022 21:06:42 schrieb Thiede, Christoph christoph.thiede@student.hpi.uni-potsdam.de: Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed. With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
> [...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...] The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
* [self halt] ensure: [1 second wait]. "Let the user know that something is happening"
* [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)"
* [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here."
* [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz ==================== Summary ==================== Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160 Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092. The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode. Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail). Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107 Please review as I'm no debugger expert and this is just my suggestion. Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox) =============== Diff against Tools-ct.1160 =============== Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'! !Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context. Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery. * A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes. If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world. --- In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.! Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier" + terminationMethod := #terminateAggressively. + self close + ! - self close.! Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize super initialize. Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess]. externalInterrupt := false. selectingPC := true. + contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.! Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process" + terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.! Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher." contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil. interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil. Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Christoph, hi Jaromir --
Reverted via Tools-mt.1163. However, we shoul now fix that failing test #test20TerminateProcess.
Best, Marcel Am 14.06.2022 10:44:19 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
In your example, one would just have to #yield the UI process for the moment within #windowIsClosing. ;-)
I will revert that for now.
Best, Marcel Am 13.06.2022 21:06:42 schrieb Thiede, Christoph christoph.thiede@student.hpi.uni-potsdam.de: Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed. With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
> [...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...] The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
* [self halt] ensure: [1 second wait]. "Let the user know that something is happening"
* [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)"
* [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here."
* [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz ==================== Summary ==================== Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160 Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092. The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode. Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail). Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107 Please review as I'm no debugger expert and this is just my suggestion. Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox) =============== Diff against Tools-ct.1160 =============== Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'! !Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context. Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery. * A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes. If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world. --- In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.! Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier" + terminationMethod := #terminateAggressively. + self close + ! - self close.! Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize super initialize. Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess]. externalInterrupt := false. selectingPC := true. + contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.! Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process" + terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.! Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher." contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil. interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil. Smalltalk installLowSpaceWatcher. "restart low space handler"!
Ha, at least now tests 21 through 24 work now :)
Yeah, debugger tests are still a pretty unexplored area for me… I haven’t figured out how exactly test 20 is designed and why fails.
Thanks, Marcel Best, Jaromir
From: Marcel Taeumelmailto:marcel.taeumel@hpi.de Sent: Tuesday, June 14, 2022 10:51 To: squeak-devmailto:squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
Hi Christoph, hi Jaromir --
Reverted via Tools-mt.1163. However, we shoul now fix that failing test #test20TerminateProcess.
Best, Marcel
Am 14.06.2022 10:44:19 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
In your example, one would just have to #yield the UI process for the moment within #windowIsClosing. ;-)
I will revert that for now.
Best, Marcel
Am 13.06.2022 21:06:42 schrieb Thiede, Christoph christoph.thiede@student.hpi.uni-potsdam.de:
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
* [self halt] ensure: [1 second wait]. "Let the user know that something is happening" * [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" * [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." * [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel
Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel
Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel
Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
* A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
---
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier"
+ terminationMethod := #terminateAggressively. + self close + ! - self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
+ contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process"
+ terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Marcel, Jaromir,
thanks for your reactions. I think it is good to have the #fork removed for now, but of course, we can continue this debate at any time.
In your example, one would just have to #yield the UI process for the moment within #windowIsClosing. ;-)
In what example? I don't think this will work if any ensure block contains a delay or yield.
the thing is the debugger runs in the UI and when it waits until the termination is finished then from the user perspective the UI is frozen.
And I argued that this i as good thing because it communicates that termination is not yet finished. :-)
We don't do a lot of concurrency/multiprocessing in Squeak for good reasons. Otherwise, you could also argue that do-its/print-its etc. should run in a background process, or that Proceeding from a debugger should run in parallel. But this would be a quite different interaction model and be a source of confusion if the user code is not designed to be executed in another process. If we really wanted to go that step to keep the debugger responsive all the time, we should at least be consistent for that and run every step in another process as well. But this definitely needs more discussion. :-)
Nothing to do with that; the issue here is tests 21-24 simply have to be fixed, my bad :) (Marcel, apologies, please let’s remove tests 21-24 before I fix them)
On my end, all DebuggerTests pass again since we have removed the #fork and I have updated #test20TerminateProcess.
Anyway, how about to use the indirect termination in #windowIsClosing for 'terminate process' mode only? The mode is hidden well enough so it’s basically for my convenience :D
Wouldn't this be more inconsistent and confusing? :-)
Best, Christoph
________________________________ Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Jaromir Matas mail@jaromir.net Gesendet: Dienstag, 14. Juni 2022 11:04:16 An: The general-purpose Squeak developers list Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
Ha, at least now tests 21 through 24 work now :)
Yeah, debugger tests are still a pretty unexplored area for me… I haven’t figured out how exactly test 20 is designed and why fails.
Thanks, Marcel Best, Jaromir
From: Marcel Taeumelmailto:marcel.taeumel@hpi.de Sent: Tuesday, June 14, 2022 10:51 To: squeak-devmailto:squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
Hi Christoph, hi Jaromir --
Reverted via Tools-mt.1163. However, we shoul now fix that failing test #test20TerminateProcess.
Best, Marcel
Am 14.06.2022 10:44:19 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
In your example, one would just have to #yield the UI process for the moment within #windowIsClosing. ;-)
I will revert that for now.
Best, Marcel
Am 13.06.2022 21:06:42 schrieb Thiede, Christoph christoph.thiede@student.hpi.uni-potsdam.de:
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
* [self halt] ensure: [1 second wait]. "Let the user know that something is happening" * [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" * [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." * [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel
Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel
Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel
Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
* A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
---
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier"
+ terminationMethod := #terminateAggressively. + self close + ! - self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
+ contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process"
+ terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
On Jun 13, 2022, at 12:06 PM, Thiede, Christoph Christoph.Thiede@student.hpi.uni-potsdam.de wrote:
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
+1000. And I suspect it will make it very difficult to port forward Croquet/Virtend which is highly concurrent and makes assumptions about the lack of concurrency in the underlying Squeak substrate. Making process termination asynchronous and concurrent is, IMNERHO, a very very bad idea.
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
[self halt] ensure: [1 second wait]. "Let the user know that something is happening" [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel
Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel
Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel
Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de:
Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org: Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger
- instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod'
instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
- A recursive error is detected.
- The current project is queried for an isolationHead
- Changes in the isolationHead are revoked
- The parent project of isolated project is returned to
- The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon
- "close the debugger and terminate the debugged process"
- "abandon the debugger from its pre-debug notifier"
- terminationMethod := #terminateAggressively.
- self close
- !
- self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
- contextStackIndex := 0.
- terminationMethod := #terminateAggressively "debugger's default termination method"!
- contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess
"close the debugger and terminate the debugged process"
terminationMethod := #terminate.
self close
!
- interruptedProcess terminate.
- interruptedProcess := nil.
- self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self].
- [interruptedProcess perform: terminationMethod.
- interruptedProcess := nil] fork.
interruptedProcess terminateAggressively.
interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
Hi Christoph, Marcel, Eliot, all,
We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
+1000. And I suspect it will make it very difficult to port forward Croquet/Virtend which is highly concurrent and makes assumptions about the lack of concurrency in the underlying Squeak substrate. Making process termination asynchronous and concurrent is, IMNERHO, a very very bad idea.
Yes, I agree 100% and apologize for an insufficient analysis on my part and opening this “can of worms” :) Thanks to Christoph for recognizing this.
In the meantime I’ve realized the problem of unresponsive UI when terminating in examples like this:
[self halt] ensure: [self error]
has a deeper root cause *not* related to termination as such and hence the change I proposed didn’t solve it entirely anyway.
The root cause why the UI freezes in the above example is the same why the UI freezes in this example:
s := Semaphore new. [1/0. s signal] fork. s wait
It happens whenever the UI is blocked (other than blocked by a delay semaphore when pausing between cycles). I’ve opened a new thread (‘Freezing UI issue’) to discuss this and would very much appreciate any feedback.
Best,
--
Jaromír Matas
mail@jaromir.net
From: Eliot Mirandamailto:eliot.miranda@gmail.com Sent: Saturday, July 2, 2022 19:41 To: The general-purpose Squeak developers listmailto:squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
On Jun 13, 2022, at 12:06 PM, Thiede, Christoph Christoph.Thiede@student.hpi.uni-potsdam.de wrote:
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
+1000. And I suspect it will make it very difficult to port forward Croquet/Virtend which is highly concurrent and makes assumptions about the lack of concurrency in the underlying Squeak substrate. Making process termination asynchronous and concurrent is, IMNERHO, a very very bad idea.
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
· [self halt] ensure: [1 second wait]. "Let the user know that something is happening" · [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" · [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." · [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel
Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel
Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel
Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de: Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org:
Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger + instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod' - instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
* A recursive error is detected. * The current project is queried for an isolationHead * Changes in the isolationHead are revoked * The parent project of isolated project is returned to * The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
---
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon + "close the debugger and terminate the debugged process" - "abandon the debugger from its pre-debug notifier"
+ terminationMethod := #terminateAggressively. + self close + ! - self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
+ contextStackIndex := 0. + + terminationMethod := #terminateAggressively "debugger's default termination method"! - contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess + "close the debugger and terminate the debugged process"
+ terminationMethod := #terminate. + self close + ! - interruptedProcess terminate. - interruptedProcess := nil. - self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self]. + [interruptedProcess perform: terminationMethod. + interruptedProcess := nil] fork. - interruptedProcess terminateAggressively. - interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
On Jul 2, 2022, at 1:48 PM, Jaromir Matas mail@jaromir.net wrote:
Hi Christoph, Marcel, Eliot, all,
We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
+1000. And I suspect it will make it very difficult to port forward Croquet/Virtend which is highly concurrent and makes assumptions about the lack of concurrency in the underlying Squeak substrate. Making process termination asynchronous and concurrent is, IMNERHO, a very very bad idea.
Yes, I agree 100% and apologize for an insufficient analysis on my part and opening this “can of worms” :) Thanks to Christoph for recognizing this.
In the meantime I’ve realized the problem of unresponsive UI when terminating in examples like this:
[self halt] ensure: [self error]has a deeper root cause *not* related to termination as such and hence the change I proposed didn’t solve it entirely anyway.
The root cause why the UI freezes in the above example is the same why the UI freezes in this example:
s := Semaphore new. [1/0. s signal] fork. s wait
It happens whenever the UI is blocked (other than blocked by a delay semaphore when pausing between cycles). I’ve opened a new thread (‘Freezing UI issue’) to discuss this and would very much appreciate any feedback.
Cool. I’ll try and be present for this conversation; it’s an important issue.
Best,
-- Jaromír Matas mail@jaromir.net
From: Eliot Miranda Sent: Saturday, July 2, 2022 19:41 To: The general-purpose Squeak developers list Subject: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
On Jun 13, 2022, at 12:06 PM, Thiede, Christoph Christoph.Thiede@student.hpi.uni-potsdam.de wrote:
Hi Marcel,
thank you for the reply!
We could not think of a serious issue to use a helper process for termination.
Let's construct a simple example:
m := Morph new. [m halt] ensure: [m delete].
"From the debugger, abandon the process."
In the past, first, the debugged process was unwound and after that, the UI was resumed.
With the new change, both will happen concurrently. As Morphic is in general not concurrency-safe, this clearly violates the execution semantics from the debugger IMO.
It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
I don't see any concurrent UI operations during the opening and closing of the Morphic debugger itself. Also, everything which happens concurrently here is under our explicit control, whereas we cannot know what any domain-specific logic in any ensure block of the debugger process might do.
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
But that is very much more indirect and often unseen feedback. In 99% of all cases, unwinding will happen without any noticeable delay. In the remaining 1%, I argue that there is a high chance that something exceptional and interesting is happening - for instance, some very expensive clean-up code, a deadlock, or some modal operation (see also the examples in the appendix). It seems natural to me that "UI is blocked after pressing terminate" indicates that "termination has not yet completed". I think that users will benefit more from that direct feedback than that they will be glad about "continuing their work a tiny fraction of a second earlier". We have been very hesitant to use concurrency in general in Squeak until yesterday, and I think this has preserved us from another dimension of complexity and a large number of hard-to-debug issues. (The only exception that comes to my mind are things like Shout background styling with a very small scope). Especially, we have never started to distribute user code to multiple processes, which we are effectively doing with this change. I insist that this will open a can of worms and will lead us into something like "concurrency hell".
+1000. And I suspect it will make it very difficult to port forward Croquet/Virtend which is highly concurrent and makes assumptions about the lack of concurrency in the underlying Squeak substrate. Making process termination asynchronous and concurrent is, IMNERHO, a very very bad idea.
Coming from the other side, what is the greatest advantage that this fork could do for us? I could not find any pointer on this in the referenced thread (The Trunk: ToolsTests-ct.107.mcz). The only argument seems to be in the version message:
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
So are we talking about something like this?
[self halt] ensure: [self error].
"Terminate from halt, then proceed from error"
Indeed this requires a user interrupt before seeing the second debugger if we don't fork in #windowIsClosing. However, as this does not happen when you #abandon instead of #terminate, I would rather suspect the new implementation of #terminate here (which I have not yet found the time to study). If this is the only reason for the #fork, I would consider the #fork a workaround for an issue in #terminate. From the perspective of getting the release done, I don't think that this special case (which is anyway kind of hidden via the debugger's window menu) should be prioritized higher than the negative consequences I mentioned above. Also, we don't support this case very well at all at the moment: if you abandon the second debugger, a MessageNotUnderstood: UndefinedObject>>stepToCallee will be raised from Context>>runUntilErrorOrReturnFrom:. Something else to investigate. :-) (PS: Note to myself: We should have one or two explicit DebuggerTest for this situation.)
Also, note that the new implementation of #windowIsClosing leads to two dangling processes from running all DebuggerTests (nil MNU for interruptedProcess in #windowIsClosing) despite all tests are passing.
tl;dr: I strongly vote for removing that #fork for two reasons: unexpected/unmanageable concurrency in user code, and a bad trade-off between beauty and informativeness.
But I'm always open to your arguments. :-)
Best, Christoph
Appendix 1 - further observations for terminate/abandon from debugger and comments:
· [self halt] ensure: [1 second wait]. "Let the user know that something is happening" · [self halt] ensure: [self inform: #foo]. "! When should the debugger window disappear? If we had a proper hook for this in SystemWindow, I think that the termination should take place even BEFORE the window is actually deleted. This would make the aforementioned feedback even easier to grasp. (Asked differently, why is #windowIsClosing sent when the window has been deleted already? Shouldn't it be rather named #windowHasClosed then?)" · [self halt] ensure: [[Project current world doOneCycle. Processor yield ] repeat]. "Do it, abandon, interrupt, abandon, then watch the process browser. Not a regression, but we have a stale process at [] in Context>>resume: here." · [self halt] ensure: [Transcript showln: Processor activeProcess = Project uiProcess]. "Outputs false after abandon. Not a regression, but this looks suspicious to me. I think we should temporarily set Project >> #uiProcess: back to the interruptedProcess in #windowIsClosing. Unfortunately, this only works in Morphic."
Von: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org im Auftrag von Taeumel, Marcel Gesendet: Montag, 13. Juni 2022 14:50 Uhr An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-jar.1161.mcz
That said, if we discover a serious issue during the next days, we can always make process termination in the debugger synchronous again.
Best, Marcel Am 13.06.2022 14:49:05 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
[...] this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. [...]
The Process Browser will show this very easily later without disturbing the user's flow for the moment. :-)
Best, Marcel Am 13.06.2022 14:48:01 schrieb Marcel Taeumel marcel.taeumel@hpi.de:
Hi Christoph --
I double-checked with Tom (tobe) on this. We could not think of a serious issue to use a helper process for termination. It's not quite different to what the debugger itself does and we also use helper processes to, for example, handle CMD+Dot.
Best, Marcel Am 13.06.2022 14:03:13 schrieb Christoph Thiede christoph.thiede@student.hpi.uni-potsdam.de:
Hi Jaromir,
sorry for not responding earlier!
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Could you elaborate further on that? I have a bad feeling with it. If the termination behaves suspiciously and takes a longer time, or the process never ends (e.g. via a non-local return), this was visible in the past because the UI was blocked. Now it's no longer possible to see this directly. Furthermore, anything might happen during the termination which now happens in a separate process, opening a potential can of worm of concurrency issues (see also "async hell"). Couldn't we simply keep the termination running synchronously? User interrupt will still work if something goes wrong with the termination, but at least the user will notice.
Best, Christoph
Am 13.06.2022 09:42 schrieb commits@source.squeak.org: Marcel Taeumel uploaded a new version of Tools to project The Trunk: http://source.squeak.org/trunk/Tools-jar.1161.mcz
==================== Summary ====================
Name: Tools-jar.1161 Author: jar Time: 9 June 2022, 1:57:46.549529 pm UUID: f3921b2b-5628-104d-98a9-1e99671344a2 Ancestors: Tools-ct.1160
Fix an inconsistent Debugger behavior WRT the new alternative abandon/terminate/destroy modes introduced by Kernel-ct.1434 and Tools-ct.1092.
The suggestion is to make all three termination methods equal in terms of how the chosen termination mode is propagated to #windowIsClosing. Current asymmetrical approach leads to an incorrect debugger termination via #terminate mode.
Replace termination of the debugged process with an indirect termination via a forked process (otherwise the debugger waits until the debugged process is terminated but if another error is raised during unwind the termination is halted and the assertion fail).
Following up a conversation in http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-May/220675.html
Complemented by ToolsTests-jar.111 fixing the failing/incorrects tests in ToolsTests-ct.107
Please review as I'm no debugger expert and this is just my suggestion.
Superseeds (or replaces) Tools-jar.1159 (please kindly remove from Inbox)
=============== Diff against Tools-ct.1160 ===============
Item was changed: CodeHolder subclass: #Debugger
- instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression terminationMethod'
instanceVariableNames: 'interruptedProcess contextStack contextStackIndex contextStackList receiverInspector receiverInspectorState contextVariablesInspector contextVariablesInspectorState externalInterrupt proceedValue selectingPC savedCursor isolationHead failedProject labelString message untilExpression' classVariableNames: 'ContextStackKeystrokes ErrorReportServer FullStackSize InterruptUIProcessIfBlockedOnErrorInBackgroundProcess NotifierStackSize SavedExtent StackSizeLimit WantsAnnotationPane' poolDictionaries: '' category: 'Tools-Debugger'!
!Debugger commentStamp: 'mt 12/17/2019 12:19' prior: 0! I represent the machine state at the time of an interrupted process. I also represent a query path into the state of the process. The debugger is typically viewed through a window that views the stack of suspended contexts, the code for, and execution point in, the currently selected message, and inspectors on both the receiver of the currently selected message, and the variables in the current context.
Special note on recursive errors: Some errors affect Squeak's ability to present a debugger. This is normally an unrecoverable situation. However, if such an error occurs in an isolation layer, Squeak will attempt to exit from the isolation layer and then present a debugger. Here is the chain of events in such a recovery.
- A recursive error is detected.
- The current project is queried for an isolationHead
- Changes in the isolationHead are revoked
- The parent project of isolated project is returned to
- The debugger is opened there and execution resumes.
If the user closes that debugger, execution continues in the outer project and layer. If, after repairing some damage, the user proceeds from the debugger, then the isolationHead is re-invoked, the failed project is re-entered, and execution resumes in that world.
In September 2019, we added MorphicDebugger and MVCDebugger to untangle framework-specific features in our debugger infrastructure. However, this is just an intermediate step. The overall goal would be to remove those two subclasses again while preserving their functionality. Mostly, MVC and Morphic differ in their GUI-process management. This means that "proceed" and "close" work differently depending on the process that is being debugged. --- One idea is to attach that framework-specific information to the process objects. See Process >> #environmentAt: and #environmentAt:put:. Also see ToolSet's #handle* and #debug* methods.!
Item was changed: ----- Method: Debugger>>abandon (in category 'context stack menu') ----- abandon
- "close the debugger and terminate the debugged process"
- "abandon the debugger from its pre-debug notifier"
- terminationMethod := #terminateAggressively.
- self close
- !
- self close.!
Item was changed: ----- Method: Debugger>>initialize (in category 'initialize') ----- initialize
super initialize.
Smalltalk at: #MessageTally ifPresentAndInMemory: [ :tally | tally terminateTimerProcess].
externalInterrupt := false. selectingPC := true.
- contextStackIndex := 0.
- terminationMethod := #terminateAggressively "debugger's default termination method"!
- contextStackIndex := 0.!
Item was changed: ----- Method: Debugger>>terminateProcess (in category 'context stack menu') ----- terminateProcess
"close the debugger and terminate the debugged process"
terminationMethod := #terminate.
self close
!
- interruptedProcess terminate.
- interruptedProcess := nil.
- self close.!
Item was changed: ----- Method: Debugger>>windowIsClosing (in category 'initialize') ----- windowIsClosing "My window is being closed; clean up. Restart the low space watcher."
contextStack := nil. receiverInspector := nil. contextVariablesInspector := nil.
interruptedProcess == nil ifTrue: [^ self].
- [interruptedProcess perform: terminationMethod.
- interruptedProcess := nil] fork.
interruptedProcess terminateAggressively.
interruptedProcess := nil.
Smalltalk installLowSpaceWatcher. "restart low space handler"!
squeak-dev@lists.squeakfoundation.org
-
Christoph Thiede -
commits＠source.squeak.org -
Eliot Miranda -
Jaromir Matas -
Marcel Taeumel -
Thiede, Christoph