Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proceed button disabled in pre-debuggers invoked from lower-priority processes #92

Open
LinqLover opened this issue Aug 20, 2023 · 4 comments
Labels
bug [WHAT] Something isn't working as expected. Automated tests beneficial. :- tools and libraries [SCOPE] From #asTextToHtml over JSON parsing to Debugger and Browser. ui system [SCOPE] Morphic, ST80 (or MVC), Etoys, ...

Comments

@LinqLover
Copy link
Contributor

LinqLover commented Aug 20, 2023

This here works as expected:

[(self halt) inspect] forkAt: 50

but this one has a UI glitch:

[(self halt) inspect] forkAt: 30

The proceed button is disabled and you have open the full debugger before being able to proceed.

This has worked in Squeak 6.0, so my first guess is that it this could be a regression from Tools-mt.1184 & Co.? @marceltaeumel?

See Debugger>>#openWithLabel:contents:fullView:. At a first look, it seems that prior to that refactoring, the debugger UI was built on the UI process but now is no longer. Without knowing the details, this looks suspicious to me - at least in Morphic, all UI code should happen in the UI process, alone for the reason of avoiding concurrency issues.

(Note that the same expectation is violated for [(self inform: 'foo') inspect] forkAt: 30 as well, but this is a different story and I hope that we can ultimately solve this with the introduction of RequestException.)

@LinqLover LinqLover added ui system [SCOPE] Morphic, ST80 (or MVC), Etoys, ... base system [SCOPE] Squeak's basic (language) concerns such as Kernel, Collections, Graphics, Network morphic [SCOPE] The issue is related to the Morphic framework. bug [WHAT] Something isn't working as expected. Automated tests beneficial. :- labels Aug 20, 2023
@marceltaeumel marceltaeumel changed the title Cannot proceed pre-debuggers invoked from lower-priority processes Proceed button disabled in pre-debuggers invoked from lower-priority processes Aug 21, 2023
@marceltaeumel
Copy link
Member

At a first look, it seems that prior to that refactoring, the debugger UI was built on the UI process but now is no longer. Without knowing the details, this looks suspicious to me - at least in Morphic, all UI code should happen in the UI process, alone for the reason of avoiding concurrency issues.

We did this to avoid strange debugger chains. The mere construction of a tool via ToolBuilder is not super critical to happen in a non-UI process. Also, a debugger is no regular tool as it interacts with process management. Even a forced but self-contained doLayout or re-drawing should be fine. Hmm... maybe we could modify MorphicProject >> #openDebuggerWindow: to only "open" it through the UI process but test-layout+test-draw it in that process-to-debug we are currently in.

(Note that the same expectation is violated for [(self inform: 'foo') inspect] forkAt: 30 as well, but this is a different story and I hope that we can ultimately solve this with the introduction of RequestException.)

Well, you cannot always guard against client code invoking UI stuff in a forked process. Your example is just similar to [ Morph new openInWorld] forkAt: 30 or [self currentWorld inspect] forkAt: 30. This is just bad style. Nothing we can or should fix.

@marceltaeumel marceltaeumel added tools and libraries [SCOPE] From #asTextToHtml over JSON parsing to Debugger and Browser. and removed base system [SCOPE] Squeak's basic (language) concerns such as Kernel, Collections, Graphics, Network morphic [SCOPE] The issue is related to the Morphic framework. labels Aug 21, 2023
@marceltaeumel
Copy link
Member

Note that openDebuggerWindow: works with windowOrModel because of the SyntaxError special case.

@marceltaeumel
Copy link
Member

I checked. Having the tool builder construct (and open) debuggers from a non-UI process (in the case of Morphic) is to prevent debugger chains and have a more reliable detection of recursive errors.

In this issue/regression here, it is about a mere UI glitch. Inconvenient, but not a serious bug. It's a trade-off. Having a reliable detection of recursive errors is more important than this.

Still, the following items should be addressed in the future:

  • unhandled errors in high-priority processes can leave Morphic UI process in unsafe state so that tests in MorphicProject >> #openDebuggerWindow: will fail for reasons unrelated to that unhandled error
  • unhandled errors in low-priority processes exhibit a race condition for debugger-model updates #interruptedProcessShouldResume and #interrupedProcessIsReady

Well, both cases could be addressed my doing UI stuff only in the Morphic UI process but might then cause those endless debugger chains again if UI code is involved.

I think we should leave it as is for now. That low-priority-process issue here could be addressed with a hack. Not sure it is worth it.

LinqLover added a commit to hpi-swa-lab/squeak-inbox-talk that referenced this issue Nov 7, 2023
@LinqLover
Copy link
Contributor Author

Thank you for looking at this. From a user perspective, I would still like to fix this issue through a hack. Other than a UIManager/Morphic invocation, exceptions are a pretty UI-agnostic mechanism that should work in any process. IMHO it would be a pity to formally support termination and debugging only for low-priority processes but no direct resumption. The above comment documents a real instance of this issue in one of my projects. While this is just an extra click for experts, I imagine confusion in beginners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug [WHAT] Something isn't working as expected. Automated tests beneficial. :- tools and libraries [SCOPE] From #asTextToHtml over JSON parsing to Debugger and Browser. ui system [SCOPE] Morphic, ST80 (or MVC), Etoys, ...
Projects
None yet
Development

No branches or pull requests

2 participants