[Reopen] Migration of events from the debugger UI to the debugger action model to automate its updates

[adri09070]

Fixes #367

In StDebugger, the session was accessed through a session holder. So, it was subscribed to the session holder to update itself and the action model when the session state - or the session itself - had changed. However, it should not be the case, as the action model should be able to exist without a GUI. Furthermore, the action model has access to the session so it could play the role of a session holder...

As a result, the action model is now subscribed to the session and handles all events that were handled in StDebugger before and StDebugger is now subscribed to the action model, which notifies the GUI when it has to update.
This migration of events was also the occasion to remove unused update hooks related to these updates.

Some Sindarin commands used to unsubscribe the debugger from the session to perform several actions and update the debugger only once. This is now handled by an "update guard" system in StDebuggerActionModel, notably thanks to the preventUpdatesDuring: method.

The handling of the MethodAdded event from the SystemAnnouncer, has been migrated from StDebuger to StDebuggerActionModel for the same reasons.

It's for now the same as the PR #368 that has been reverted because unstable. I reopen it the PR to fix it

[adri09070]

So, the CI is green here... I'm confused and don't know how to deal with the problem

[MarcusDenker]

I sadly have no idea...

[MarcusDenker]

maybe we can try again to merge it and then analyse the problem on the other CI.. but from my side this has to wait till after August 15

[adri09070]

I investigated the problem on the build artefact you gave me and found out that there's still a memory leak somewhere.

I'm using NewCommandLineRunner. This is a tool in development that allows to run tests in the order we want, and that opens a debugger each time that an error or a failure occurs. It also stores the results of all tests in xml files named after the executed packages.

I discovered that there's a particular test that make the image crash. Even, when I change the execution order for test cases, it's always the same debugger test that is problematic.

But it only happens when I execute all debugger tests. If I run only the package that contains the problematic test, there's no error.

I think (and I hope) that I will manage to find where the memory leak is, and the cause of the crash (The crash is certainly the reason why all tests failed on the CI after merge) thanks to this tool.

But it's quite a shame that it's not possible to have a direct access to the other CI, without merging

[adri09070]

So here are the changes that I made to close the memory leak:

• All references to subscribed debuggers are now weak. To do that, I use a WeakKeyDictionary instead of a Dictionary to store the action model susbscribers.
• Now, when a debugger is cleared with StDebugger>>#clear, I automatically use StDebuggerActionModel>>#clear to unsubscribe the action model from its session and the system announcer, as well as unsubscribing all other debuggers that were subscribed to this action model. This prevents the action model to be used again with another debug session so this may be changed later.

Furthermore, I skipped a specific test: StDebuggerActionModelTest>>#testComputeInitialTopContext because this test looks like it causes some problem. Indeed, if I don't perform operations in a specific order (that should be performed in any order), some variables in this test become nil without being accessed and the objects that were stored in these variables are NOT garbage-collected.

For now, this test pass if it is run via the system browser but if you debug this test and perform stepOver, the image crashes before we reach the last assertion in the test.

In addition, it depends on the test runner that is used to run the test! If I use NewCommandLineRunner, developed by @aboubacardiawara, an instance variable of the NewCommandLineRunner becomes nil without being accessed, as I mentioned above.

[adri09070]

It looks like it solves the memory leak. For example, when we open a debugger, close it and save the image, then StDebugger allInstances and StDebuggerActionModel allInstances are empty.

However, when I run all tests via TestRunner, the world breaks: a lot of windows that are opened in tests aren't closed and debuggers are not garbage collected easily. For instance, if you open a debugger, close it and save the image, StDebugger allInstances and StDebuggerActionModel allInstances are not empty. To garbage collect all debuggers, we need to close the image and open it again. I don't really know if it's a debugger-related problem as the windows that stay open are playgrounds or spec example windows but not debuggers. Plus, it is also necessary to close the image to garbage collect StInspector allInstances for example

[adri09070]

Actually, if I run all tests via TestRunner on an image after the revert, I get the same problem. So, I don't think that my PR is cause of the problem

[adri09070]

If this PR is merged again, we need a plan to quickly analyse the cause of the problem to fix it.

What bothers me is that the production CI failed all tests with this error: "FileWriteError: File Tests-osx-64-NewTools-Debugger-Tests-Test.xml is closed" but I never could reproduce the problem with the CI build artefact.

As I said, a particular test fails depending on the test runner that is used to run tests. So I would like to know what tool is used by the production CI to run tests. I'm also curious to know if the production CI will work properly as I skip the problematic test I've noticed

[adri09070]

I managed to reproduce the production CI error: "FileWriteError: File Tests-osx-64-NewTools-Debugger-Tests-Test.xml is closed" with @aboubacardiawara, with the CommandLineRunner. I get the error when StDebuggerActionModelTest>>#testComputeInitialTopStack is run.

When we skip it, tests are green and we only get warnings from StDebuggerTestProvider that should be investigated in my opinion as I have the intuition that this class is linked to all our troubles. Here are the results:

Running tests in 1 Packages
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
NewUndeclaredWarning: StTestDebuggerProvider>>buildDebuggerWithMissingClassContext (MissingClass is Undeclared)
243 run, 243 passes, 0 failures, 0 errors.

I think this can be merged and that the problematic test should be skipped, and investigated later because this is not a simple bug at all

[StevenCostiou]

Do not merge this one yet we need to investigate it more to be sure.

[jecisc]

There are conflicts now

[adri09070]

Yes, I'm not surprised. This PR has been pending for too long and now, some other changes will complicate things. I will try to fix that and see with @StevenCostiou if we can integrate it (I think we can as I think I had isolated the problematic test) this month

[adri09070]

Still unrelated failing tests.

What I basically did is:

• testing that all events are propagated by the action model to a dummy debugger (steps, restart, proceed, recompile existing method, compile a new method)
• I made the update system more flexible:
  • Before, the debugger action model kept a weak key dictionary that associated to a subscriber (= a debugger) a block to be executed when any event is received (steps, restart, proceed, recompile existing method, compile a new method), so the same code is executed to update the action model and the GUI, regardless of the received event
  • Now, the debugger action model holds an announcer that announces different types of events, and the subscriber chooses what to do for each type of event. The subscriber can call different methods for different types of events (which is the case right now for the StDebugger in Pharo12). Also, the debugger action model doesn't know what the subscribers will do from these events (which is good because it shouldn't care).
  • Now, a nice thing this that the announcements hold the debugger action model that made the announcement. It can be important if a subscriber is subscribed to many action models (e.g. we could imagine a server that communicates with several remote debuggers and that is thus subscribed to several debugger action models. In this case, this is good to be able to distinguish the emitter of an announcement )

[adri09070]

So many tests are red now...

From the looks of it, I guess a PR with some debugger tests that are not well-written has been merged. I will look at that because I want to merge this PR as soon as possible.

The more we wait, the more complicated it gets to integrate it

[adri09070]

To me, everything looks good. We just need pharo-project/pharo#14670 to be merged first. This PR introduces a mechanism that should prevent ANY problem of subscription that is not cancelled, as we had in the past

[StevenCostiou]

I did a global review, more specifically on the latest changes since I already did review the previous ones, but they are much older and my memory is not fresh.

It looks good, and better than before.
I suggest we merge the PR tomorrow, and monitor its impact just in case.

[StevenCostiou]

@jecisc The failing test is an undeclared in Tonel, I thnk you said somewhere we can merge PRs with this problem?

[jecisc]

Yes and this is now fixed! :)