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

Use analysis abort in server mode #34

Merged
merged 11 commits into from
Jul 25, 2022
Merged

Use analysis abort in server mode #34

merged 11 commits into from
Jul 25, 2022

Conversation

karoliineh
Copy link
Member

This branch has the initial abort logic, relating to #20. Although the initial version works for aborting the analysis and starting a new one, it executes the code in GobPie following the reanalysis even after the abort. This results in the actions (reading the results from the socket and printing "analysis finished") being executed consecutively as many times as the analysis was restarted.

To solve this issue, we have discussed with @sim642 to try and use Java threads for the actions from triggering analyses to requesting results.

@sim642
Copy link
Member

sim642 commented May 27, 2022

Actually, the solution might be a lot simpler than threads and interrupting. If Goblint receives the aborting signal, then it still replies with a message in the socket, but the difference is that it should contain "status": "Aborted". So the GobPie code that's waiting for a response will get one (and it actually must read it, otherwise the IDs probably get mixed up), but can then decide not to separately request messages and quietly return.

@sim642 sim642 added the enhancement New feature or request label May 27, 2022
@karoliineh
Copy link
Member Author

I implemented the solution, where the result from the response is checked and reanalyze method returns in case of an abort (instead of trying to read messages and getting stuck because of it).

Although, now if the analysis is aborted, a pop-up with "GobPie finished analyzing the code." is still shown. Unfortunately, I could not find a way how to turn it off, because it seems that MagpieBridge creates this message each time the analyze method finishes, even if any results have not been given to be consumed by the server (MagpieServer.java):

if (a != null) {
      this.forwardMessageToClient(
          new MessageParams(MessageType.Info, a.source() + " started analyzing the code."));
      a.analyze(fileManager.getSourceFileModules().values(), this, rerun);
      this.forwardMessageToClient(
          new MessageParams(MessageType.Info, a.source() + " finished analyzing the code."));
    }

which is a bit odd behaviour, because we do not really finish the analysis with abort (no results can be expected to be shown).

@karoliineh karoliineh marked this pull request as ready for review May 31, 2022 16:22
@sim642
Copy link
Member

sim642 commented May 31, 2022

One incredibly stupid way to prevent the "finished analyzing the code" message from being shown would be to have Thread.sleep(Long.MAX_VALUE); before our implementation of analyze returns (if the read status was aborted).
Although that might have some bad effect by leaving a large number of threads around waiting (practically) forever.

@michael-schwarz
Copy link
Member

michael-schwarz commented May 31, 2022 via email

src/main/java/analysis/GoblintAnalysis.java Outdated Show resolved Hide resolved
src/main/java/analysis/GoblintAnalysis.java Outdated Show resolved Hide resolved
src/main/java/analysis/GoblintAnalysis.java Outdated Show resolved Hide resolved
@sim642
Copy link
Member

sim642 commented May 31, 2022

This sounds like exactly the sort of thing where one might want to file a bug report / do a PR upstream, doesn't it?

Sure! An upstream solution probably needs to be a bit more thought through though since there might be more than two possible results an analyzer might want to return. But an upstream issue definitely wouldn't hurt.

@karoliineh
Copy link
Member Author

I realized that the analyze method was indeed itself blocking, and it had nothing to do with Magpie's threading #35.

I now run the process of communicating with Goblint in a separate thread, so that the analyze method itself will finish right away and not block the next call. When there is another call for analyze (triggered by saving a file), and the previous analysis is running, it sends the SIGINT to Goblint and kills the process that is running in the separate thread.

With this the pop-up situation:

Although, now if the analysis is aborted, a pop-up with "GobPie finished analyzing the code." is still shown.
...
which is a bit odd behaviour, because we do not really finish the analysis with abort (no results can be expected to be shown).

is even worse, because the analyze method finishes instantly and the pop-up will say "GobPie finished analyzing the code.", although the analysis is still running. However, we agreed in the PRIDE workshop that we will remove the default pop-up messages from MagpieBridge. A PR for that is not created yet, but this will be eventually solved.

Therefore, this PR should now be ready for merge @sim642.

@sim642
Copy link
Member

sim642 commented Jun 22, 2022

I realized that the analyze method was indeed itself blocking, and it had nothing to do with Magpie's threading #35.

It still does to the extent that if Magpie used an unbounded thread pool for its requests, then it would've worked all along since Magpie itself would have been able to immediately start all new requests in new threads without waiting for the old blocking ones.
Although overall it's more reliable anyway to manage our own threads and not rely on internal implementation details of Magpie thread pools.

@karoliineh karoliineh merged commit 95adf7a into master Jul 25, 2022
@sim642 sim642 deleted the analysis-abort branch September 12, 2022 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants