this is extremely annoying, now I have to stop debug session after any new project opened to be able to do the development

I'm not aware of what might block the scanning.
I'll investigate...

This can not be a debugger bug.
It can be reproduced by simply running the application and changing the APIs before it finishes.
It behaves the same way under debug and under run.

The ant support blocks reindexing by calling:
org.netbeans.modules.project.indexingbridge.IndexingBridge.runProtected

I will attach a thread dump. Note that I was able to reproduce on a project with CoS *disabled*.

Created attachment 118548 [details]
Thread dump.

The behavior is intentional - scanning is disabled while builds are in progress, to avoid excessive rescans due to bulk changes like classpath entries being created. Whether this suppression should really apply also to user edits made via FileObject.outputStream, I am not sure, but that is apparently what IndexingManager.runProtected does.

Note that in bug #208213 comment #0 I requested a different API that would allow explicit start and end calls, that could be matched for example with the logic in the Ant executor which sets the progress bar to indeterminate mode if some time elapses with no new build events or messages. But that was rejected in comment #1, and the current API only supports passing a thunk, which is not flexible enough for this purpose. (Hackish workaround, circumventing the intent of the runProtected API: start a new thread whose only function is to call rP and then wait on a semaphore which other threads toggle.)

>Whether this suppression should really apply also to user edits
>made via FileObject.outputStream, I am not sure, but that is apparently what
>IndexingManager.runProtected does.
Yes it should, it blocks all indexing events (projects, visibility, api, file events, etc).
There is no difference from FileEvents got from external modification or FO.getOutputStream.

I am still against an API which may cause unbalanced locking. Only the API where language (synchronized) or the framework (runnable) guaranties the lock balance is safe, but can be put to some friend API.
The problem is that the project.indexingbridge blocks when project runs. Do you suggest to unlock the indexing in AntLoger before run?

(In reply to comment #7)
> I am still against an API which may cause unbalanced locking.

Then I think no fix is possible.

> Do you suggest to unlock the indexing in AntLogger before run?

No, only if there are no new events for a while, as would be true for most of the duration of a typical run/debug session. See ant.module.bridge.impl.NbBuildLogger.sleepTask and maven.execute.AbstractOutputHandler.sleepTask. When the handle is unsuspended (switched to determinate or indeterminate), the indexing would be locked again. Of course there may be multiple processes running in parallel, and even in a single process there is no natural dynamic scope for locking or unlocking (other than the entire duration of the process as in the current code) - which is why the API has to support a semaphore.

(In reply to comment #8)
> (In reply to comment #7)
> > I am still against an API which may cause unbalanced locking.
> 
> Then I think no fix is possible.

As the current situation seems worse than the pre-7.2 state, I think this should be reverted unless it can be fixed.

> 
> > Do you suggest to unlock the indexing in AntLogger before run?
> 
> No, only if there are no new events for a while, as would be true for most of
> the duration of a typical run/debug session. See

Such heuristic may or may not work reasonably, I suggest to test it thoroughly and actively evaluate if it fits well into the user's workflow.

The public API has to be self defensive.
I already have problems with clients of IndexingManager.refreshIndex() causing rescans of all open projects even when single file has changed. A public semaphore like API which affects all language supports and keeps responsibility on clients seems to me as too much of adrenaline. But I am OK with a friend API with limited number of friends (all inside NB repository to be able to review them).

I've looked at NbBuildLogger.sleepTask. It's probably OK for suspending the Progress which is later finished anyway. But using it to balance a semaphore will be non trivial. The checkForStop does not guarantee that switchToIndeterminate was invokes as many times as suspend as it's not needed but it will be needed for a semaphore.

(In reply to comment #10)
> I am OK with a friend API with limited number of friends

Such as project.indexingbridge? But it has no API in parsing.api it could call. I suppose project.indexingbridge could define some kind of SPI, implemented in parsing.api to call IndexingController.enter/exitProtectedMode.

> using it to balance a semaphore
> will be non trivial. The checkForStop does not guarantee that
> switchToIndeterminate was invoked as many times as suspend

Of course, but this should not be hard. The task just needs to keep an AtomicBoolean marking whether it is currently sleeping. When that state changes, the semaphore is incremented or decremented accordingly. And of course when the handle is finished (which we know happens because otherwise you could see it in the status bar) then the semaphore is released if it was locked. In other words, for each process - which is 1-1 with a sleepTask - there is just a simple boolean state; the full counting semaphore is inside project.indexingbridge, and global.

Created attachment 118589 [details]
Proposed patch

Taking lock from different thread than releasing it is a bad practice.
I would at least change the enterProtectedMode(@NullAllowed Long id) and exitProptectedMode(@NullAllowed Long id) to:

@NonNull
Lock  enterProtectedMode();

interface Lock {  
    void release();
}

to make it safer, passing null to enterProtectedMode(@NullAllowed Long id) is strange.
Otherwise OK.

If you want I can change it.

(In reply to comment #13)
> Taking lock from different thread than releasing it is a bad practice.

java.util.concurrent.locks.Lock has no such restriction.

> change the enterProtectedMode(@NullAllowed Long id) and
> exitProtectedMode(@NullAllowed Long id) to:
> 
> @NonNull Lock enterProtectedMode();
> interface Lock {  
>     void release();
> }

Can do that, but what about the IndexingBridge friend API - leave as is, or make a similar change?

>java.util.concurrent.locks.Lock
I've thought some custom Lock class, but I've used wrong name :-( sorry.
It can be whatever just some identity I can later keep the caller stack trace in it.
>Can do that,
Thanks!
>but what about the IndexingBridge friend API - leave as is, or
make a similar change?
You mean the Versioning IndexingBridge? If so we can keep it as it is, it can be rewritten later on if needed.

(In reply to comment #16)
>> what about the IndexingBridge friend API
>
> You mean the Versioning IndexingBridge?

No, org.netbeans.modules.project.indexingbridge.IndexingBridge. You suggested changing how RepositoryUpdater.Worker is accessed - using a lock-based method rather than enterProtectedMode(null). I was asking whether you require IndexingBridge (and thus also IndexingBridgeImpl) to be accessed via lock-based method, or whether its current API is acceptable.

I looked into using Lock in Worker. It would be significantly more complicated, since IndexingBridge would also have to expose its own Lock interface (otherwise IndexingBridgeImpl would not know which internal lock needed to be unlocked), with a proxy to the internal lock, and the Ant and Maven modules would have to keep not just a friend Lock but still the AtomicBoolean protectedMode (or other form of synchronization). Then protectedOwners could no longer be a List<Long> but would need to be a List<Lock>, with Controller keeping a ThreadLocal<Stack<Lock>> to satisfy the IndexingController API, isProtectedModeOwner moved to Controller (checking !stack.isEmpty()), and isInProtectedMode moved somewhere else (Controller also?) and not sure how to implement. By the time you are done I think a fair amount of stuff would have to be rewritten; not sure what the public API boundary of parsing.api really is, since it defines an implementation version, ominously.

Since Beta is approaching, I am committing the existing patch, since it is relatively straightforward and seems to work fine. If you feel strongly about the Lock design, I can try to rewrite it later.

core-main #d7a181d067de

I think the current patch is broken:
1. it provides no way to find out who locked the indexing but did not unlock it correctly. Given that any such occurrence is a P1, such lack of debugging facilities is devastating. There must be at least a log message with a stack trace on each enter and exit (level==FINE would probably suffice). P1 until then.
2. the enterProtectedMode and exitProtectedMode methods in AbstractOutputHandler do not ensure that exit will be called only after enter, even though they pretend to do so. As a side note, the code that calls the enter|exitPM methods seems pretty scary to me, considering the consequences of not unlocking the RU. A Lock object would probably enforce structure that would make the code a little bit less scary.
3. returning some kind of Lock object from enter provides two critical features:
a) debugging: the Lock can keep a stack trace of the locker, so that the bogus caller is traceable
b) error recovery: if the caller forgets to call exit/unlock, and the IDE can still be unlocked eventually if the Lock becomes GCable
4. I didn't check the code deeply, but I guess the Locks do not need to be hardwired into RepositoryUpdater, they can simply be API lipstick, invoking the standard primitives in RU.

For the beta it may be OK (except the AtomicBoolean problem described below) but I will log the thread dumps when the enterProtectedMode(null) or exitProtectedMode(null).

Even it's a friend API, the final state probably deserves an API review.

The usage of AtomicBoolean in NbBuildLogger is suspicious. The Atomics have similar semantics as volatiles, they cannot participate in invariants with other critical state variables.

For the methods:

private void enterProtectedMode() {
     if (protectedMode.compareAndSet(false, true)) {
         IndexingBridge.getDefault().enterProtectedMode();
     }
}

private void exitProtectedMode() {
     if (protectedMode.compareAndSet(true, false)) {
         IndexingBridge.getDefault().exitProtectedMode();
     }
}

performed by threads T1, T2 you can get the following "as in serial order":

T1: protectedMode.compareAndSet(false, true)
T2: protectedMode.compareAndSet(true, false)
T2: IndexingBridge.getDefault().exitProtectedMode()
T1:IndexingBridge.getDefault().enterProtectedMode();

which causes RU to block forever.

core-main #b3bbbce083ac

Up to you what you want in Beta - state prior to bug #208213, state after #208213 but before this, initial d7a181d067de, revised b3bbbce083ac, something else...

The b3bbbce083ac seems perfect to me.
I vote for it.

OK, I can graft b3bbbce083ac to the beta branch today.

(In reply to comment #25)
> OK, I can graft b3bbbce083ac to the beta branch today.

Yes, please graft right now, release72_beta has been created. The beta build will be started in 3h.

Thanks Jesse.

releases #4ba29c9dad1b

Integrated into 'main-golden', will be available in build *201204260400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/d7a181d067de
User: Jesse Glick <jglick@netbeans.org>
Log: #211005: suppress indexing only while builds are active.

I was thinking about this recently, and I am not sure if the current solution is an optimal one. Here is what I think would be better for Maven projects (I do not remember many complaints about this for ant projects):
The Maven project would cache the results of all the "standard" queries - a query invocation would simply return the value from the cache very quickly, without blocking the caller. The cache would be initially filled on project opening (during the "Opening Projects" progress) and updated at the end of a build (covered by the "Build" handle), or on background when the project metadata would change on disk. The new values would be set to the cache quickly and atomically, firing events in correct order but outside any lock. But the most important part is that the classpath reported by the project would not list entries (jars) that are physically missing on disk. The net effect would be that at the end of the "priming" build, the CP would be updated once to the correct/new values, and the scanning would be performed once, without playing tricks with stopping the indexing. Would also improve overall experience when working with maven projects, IMO.

I guess you are really commenting on bug #208213?

(In reply to comment #30)
> I am not sure if the current solution is an optimal one.

No, it is not intended to be optimal, just better than what we had.

(The most robust - not the most efficient - system would be for the scanning infrastructure to cancel a scan if it receives a countermanding event in the middle of a scan, and use an exponential backoff heuristic before restarting. The most efficient - but not robust - system would be to monitor I/O activity and only scan when it was otherwise low, as the Beagle desktop indexer attempted to do.)

> I do not remember many complaints about this for ant projects

Probably the people would be affected by this kind of problem would already be using Maven. But you would run into something similar if using Ivy + autoproject, etc.; it is not really limited to Maven projects.

> The Maven project would cache the results of all the "standard" queries - a
> query invocation would simply return the value from the cache very quickly,
> without blocking the caller.

Not much different from the current state - it uses the offline embedder, so common queries basically just read the in-memory model.

> The cache would be initially filled on project
> opening (during the "Opening Projects" progress) and updated at the end of a
> build (covered by the "Build" handle), or on background when the project
> metadata would change on disk.

Again this is what already happens.

> the classpath reported by the project would not
> list entries (jars) that are physically missing on disk.

I do not think it can work this way. If nothing else, a lot of artifacts have source associations, which may exist even when the binary does not. Also you are neglecting to consider !preferSources snapshot dependencies, common in OSGi projects (for example), which can be recreated en masse during a build and ought to be scanned in just one pass. And of course dependencies can appear (or, if snapshots, change) in the local repo incrementally at any time, including during non-priming builds of other projects, or out-of-IDE builds (cf. bug #194147), or simply by the Download Dependencies context menu action.

Better for the project query to report a stable and correct result whenever it can, and just use heuristics to suppress excessively frequent reindexing.

>(The most robust - not the most efficient - system would be for the scanning
>infrastructure to cancel a scan if it receives a countermanding event in the
>middle of a scan, and use an exponential backoff heuristic before restarting.
This is already done from the beginning NB 4.0 except of exponential back off. If the indexing is running and a new event come the current work is cancelled (if the event is compatible with the event being processed) and a new work is scheduled. There is also sliding window (not EBO) to collapse CP and GlobalPathRegistry changes. But only compatible Works can cancel each other, for example CP change can cancel previous CP change or GPR change but it cannot change File change.

For NB 6.8 (parsing.api) in addition to that the Work is also able to absorb other compatible work which is big performance benefit specially in combination with FileEventLog (NB 6.9) which processes events from single atomic action at once. For example when you do 10 modifications in single source root just one Work is executed and single javac is created rather than 10 of them.

*** Bug 210726 has been marked as a duplicate of this bug. ***

Integrated into 'releases', will be available in build *201210100002* or newer. Wait for official and publicly available build.
Changeset: http://hg.netbeans.org/releases/rev/4ba29c9dad1b
User: Jesse Glick <jglick@netbeans.org>
Log: #211005: IDE does not (re)scan projects while debug or run session is active
Revised friend API uses trackable locks. Implementation still uses a simple semaphore.