Comment 1 Mikaël Barbero 2016-09-13 11:11:51 EDT

Can't do it without API breakage or introducing IRefreshMonitor2. Not a high priority right now, so I won't push a review.

Comment 2 Sergey Prigogin 2016-09-20 23:06:05 EDT

(In reply to Mikaël Barbero from comment #1)
> Can't do it without API breakage or introducing IRefreshMonitor2. Not a high
> priority right now, so I won't push a review.

The call chain seems to be:
RefreshManager.startup(IProgressMonitor)
  RefreshManager.manageAutoRefresh(boolean)
    MonitorManager.start()
      MonitorManager.monitor(IResource)
        MonitorManager.safeInstallMonitor(RefreshProvider, IResource)
          RefreshProvider.installMonitor(IResource, IRefreshResult)

Changing this call chain doesn't seem to require changing IRefreshMonitor. Am I missing something?

Comment 3 Eclipse Genie 2016-09-21 06:54:04 EDT

New Gerrit change created: https://git.eclipse.org/r/81565

Comment 4 Mikaël Barbero 2016-09-21 06:59:51 EDT

(In reply to Sergey Prigogin from comment #2)
> Changing this call chain doesn't seem to require changing IRefreshMonitor.
> Am I missing something?

I was missing something! Thanks for the heads-up. We will eventually end up having a NullProgressMonitor because RefreshManager.startup call chain is 

RefreshManager.startup(IProgressMonitor)
  Workspace.startup(IProgressMonitor)
    Workspace.open(IProgressMonitor)
      ResourcesPlugin.start(BundleContext)

And ResourcesPlugin.start calls Workspace.open(null)... 

Also, I've had to use NullProgressMonitor in MonitorManager when it gets events being a ILifecycleListener, IPathVariableChangeListener and IResourceChangeListener.

Comment 5 Eclipse Genie 2016-10-13 17:26:11 EDT

Gerrit change https://git.eclipse.org/r/81565 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=f2152ad44481651f5e43066e6f184562de9cb2a3

Comment 6 Andrey Loskutov 2017-04-17 06:57:52 EDT

(In reply to Mikaël Barbero from comment #0)
> All the call chain from RefreshManager#startup(IProgressMonitor) should be
> changed to take a progress monitor as parameter. I will push a review
> shortly.

Mickael, what was the reason to change the behavior in the MonitorManager and introduce asynchronous monitor/unmonitor calls?

https://git.eclipse.org/c/platform/eclipse.platform.resources.git/diff/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/refresh/MonitorManager.java?id=f2152ad44481651f5e43066e6f184562de9cb2a3

I don't see how this is required for adding monitors to the code, but the change in behavior is at least unexpected and probably could cause random test failures I'm investigating in bug 510494.

Comment 7 Andrey Loskutov 2017-04-17 11:45:16 EDT

(In reply to Andrey Loskutov from comment #6)
> (In reply to Mikaël Barbero from comment #0)
> > All the call chain from RefreshManager#startup(IProgressMonitor) should be
> > changed to take a progress monitor as parameter. I will push a review
> > shortly.
> 
> Mickael, what was the reason to change the behavior in the MonitorManager
> and introduce asynchronous monitor/unmonitor calls?
> 
> https://git.eclipse.org/c/platform/eclipse.platform.resources.git/diff/
> bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/refresh/
> MonitorManager.java?id=f2152ad44481651f5e43066e6f184562de9cb2a3
> 
> I don't see how this is required for adding monitors to the code, but the
> change in behavior is at least unexpected and probably could cause random
> test failures I'm investigating in bug 510494.

I've reproduced the problem from bug 510494 now, it is a direct regression from the changes above, and I plan to revert the asynchronous installing/uninstalling of the monitors now, see bug 510494 and proposed patch https://git.eclipse.org/r/95127. The patch provides a test which fails without the patch if executed on Windows. The only thing this test does is to create/delete the same project again and again. This fails in ~3 from 1000 executions because of asynchronously running monitor install/uninstall operations.

Comment 8 Mikaël Barbero 2017-04-18 04:12:00 EDT

(In reply to Andrey Loskutov from comment #6)
> (In reply to Mikaël Barbero from comment #0)
> > All the call chain from RefreshManager#startup(IProgressMonitor) should be
> > changed to take a progress monitor as parameter. I will push a review
> > shortly.

Sergey noted that installing a refresh monitor could be a long running task on big workspaces (see his comment on patch set 15 at https://git.eclipse.org/r/#/c/73007/). This bug was then about adding cancellation support and also asynchronous execution to avoid blocking ResourcePlugin.start() for too long. Note that this asynchronous installation of monitor could be removed if core.resource is initialized asynchronously (bug 501997).

Comment 9 Mikaël Barbero 2017-04-18 04:13:04 EDT

(In reply to Andrey Loskutov from comment #6)
> Mickael, what was the reason to change the behavior in the MonitorManager
> and introduce asynchronous monitor/unmonitor calls?

Wanted to reply to the comment above ^ in comment 8

Comment 10 Lars Vogel 2017-11-23 06:08:00 EST

My workspace complains that the since tag should be 3.13 for installMonitor and resetMonitors. Is the version wrong or do we need to increase it?

Comment 11 Lars Vogel 2019-09-05 04:24:03 EDT

I think this one was fixed