Fix job watching in "reimport projects"

[tsmaeder]

eclipse-jdtls/eclipse.jdt.ls#506 is fixed now, let's remove the workaround in "ReimportMavenProjectsHandler#JobChangeListener".

[bdshadow]

taking this issue

[bdshadow]

After looking at the code and changes in eclipse.jdt.ls for some time i don't see much help of these changes at the moment. Let me explain.
So now we have a job returned by updateProject method. By the time it's returned, the job is already running and it's the main problem.
The first thing came to my mind is to use org.eclipse.core.runtime.jobs.JobGroup. But job.setJobGroup(group) can be called only before scheduling a job, otherwise - IllegalStateException.
The second thing is to add listeners. Almost the same idea as it is now, but add listeners only to our jobs, not to a job manager. But the same problem here too - it might happen that by the time we add listener, the job is already finished and listener won't be triggered.

So what we can do with the job now is to join it right away, smth like:

for (IProject project : projects) {
      projectsManager.updateProject(project).join();
}

but then we'd loose the profit of parallel execution.
@mmorhun @tsmaeder wdyt? maybe i missed smth

[mmorhun]

@bdshadow another way is to combine adding listeners to the returned jobs and then checking of each job result. So, in case if adding listener operation happen after a job finish it will be removed from the queue by result checking:

for (IJob job : submittedJobs) {
    job.addJobChangeListener(...);
}

for (IJob job : submittedJobs) {
    if (job.getResult() != null) { // job is finished
        // remove the job from the queue
    }
}

But to me sounds better to do the fix together with progress introducing. It might add new aspects here.

[bdshadow]

@mmorhun i think there is a problem in this snippet, when a job is finished between these fors. It means that both thing will happen: 1) listener will be triggered 2) if (job.getResult() != null) will be true.
So there is nothing serious about it if we only use it to remove a job from the queue. But if we want to add some operations on jobs' results we must be careful not to do it twice - in a listener and in the second loop.

Also i agree that it's better to wait for progress introducing fix and do the changes simultaneously.

[mmorhun]

@bdshadow See no problem in it. Both fors should invoke the same job finish handler. Here the queue is like a monitor and we can easily determine whether job handler was invoked before. Matter of one if.

[bdshadow]

@mmorhun agree, but this if also must be inside synchronized (or one more Lock must be used, or any other synchronization methods). Because this job handler can be entered simultaneously twice. So just need to implement it carefully

[tsmaeder]

There is code very similar to what we want in LanguageServerRegisteryImpl:

    synchronized (initializedServers) {
      for (InitializedLanguageServer initialized : initializedServers) {
        requiredServers.remove(initialized.getLaunchKey());
      }
      while (!requiredServers.isEmpty()) {
        LOG.info("waiting for launched servers on thread {} : {}", thread, requiredServers);
        try {
          initializedServers.wait();
          for (InitializedLanguageServer initialized : initializedServers) {
            requiredServers.remove(initialized.getLaunchKey());
          }
        } catch (InterruptedException e) {
          Thread.currentThread().interrupt();
          return null;
        }
      }
    }

The job listener we add to every job would simply call "initilizedServers.notifyAll()", which goes on to check all jobs again. (change var names, of course)

[tolusha]

Depends on
eclipse-jdtls/eclipse.jdt.ls#490
eclipse-jdtls/eclipse.jdt.ls#522

[svor]

PRs:

• Notify clients about finish project updating che-ls-jdt#52
• Handle project updated notifications from jdt.ls #9983