This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 208213

Summary: IndexingManager.runProtected from project system operations
Product: projects Reporter: Jesse Glick <jglick>
Component: Generic InfrastructureAssignee: Jesse Glick <jglick>
Status: RESOLVED FIXED    
Severity: normal CC: anebuzelsky, mkleint, tzezula, vstejskal
Priority: P2 Keywords: API, PERFORMANCE, PLAN
Version: 7.1   
Hardware: All   
OS: All   
Issue Type: ENHANCEMENT Exception Reporter:
Bug Depends on: 210726, 211005, 250131    
Bug Blocks:    

Description Jesse Glick 2012-02-09 19:11:33 UTC
org.netbeans.modules.versioning.indexingbridge.Bridge calls IndexingManager.refreshAllIndices to avoid triggering rescans while a VCS operation is in progress. I would like to do something similar for long-running project system operations - project group switch, Ant execution, Maven execution - but this API would be awkward to use:

1. There cannot be a dep on parsing.api from these modules for architectural reasons, so three (!) new bridge modules would need to be created just to call this one method... and friend APIs introduced to work with them.

2. IndexingManager keeps track of the thread that called runProtected, apparently assuming that all file modifications it should ignore would be from within this block. But that is not true at all for the use cases I have in mind, where helper threads are doing various things, and events might be fired from native file notifications coming from a masterfs watcher thread. What is really wanted is more like a semaphore - a way to indicate that until further notice, lots of things may be changing rapidly, and do not initiate rescans.

Consider a simplified API in a more neutral module, say api.progress, of the form

/** hold on, do not rebuild any expensive caches */
void startTransaction();
/** OK, proceed */
void endTransaction();
/** true if more starts then ends outstanding */
boolean isInTransaction();
/** notify me when first transaction starts, or last ends */
void addChangeListener(ChangeListener);
void removeChangeListener(ChangeListener);
// iIT/aCL/rCL could also be replaced or supplemented with:
/** run immediately if not in xaction, or later when last xaction ends */
void runAfterTransaction(Runnable);

The indexing manager would avoid starting scans while in transaction; other IDE components could do the same, e.g. the maven module could wait to reload POMs despite getting notifications of dependency artifacts appearing in the local repo.

It seems this would not suffice for the purposes of bug #167098, which calls refreshAllIndices, i.e. versioning.indexingbridge might still be needed.

Possibly would be useful for bug #194147 - core.windows could increment the semaphore when losing focus, and decrement when regaining it. Probably do not need to disable native file notifications while in transaction, since firing a FileEvent in and of itself is not particularly expensive, but various things triggered by these notifications (especially scanning) are.
Comment 1 Tomas Zezula 2012-02-29 14:09:31 UTC
JG01: We should create a single module with "IndexingBridgeProvider" on which versioning, ant and maven will depend. The implementation of this interface will be provided by PA. Just one api or friend-api module will be added and the original versioning bridge will be removed.

JG02: No. It does not use the threadIds to block only the changes form given thread. The thread id is used for checking that enterProtectedMode and exitProtectedMode are paired (the same thread has called enter and exit) and to assert that thread which called runProtected does not do ParserManager.parseWhenScanFinished().get() which will cause a dead lock.
So the only difference is that instead of enter|exit method the API provider runProtected which is safer as it removes form API client the responsibility to unlock the RU.

Regarding the proposed api:

>void startTransaction();
>void endTransaction();
I personally prefer the version with Runnable. We had beginTrans, endTrans in MOF. The API clients did not call the endTrans and it was real pain to find it. The runProtected in public API ensures the balanced locking. 

>boolean isInTransaction()
OK


>void addChangeListener(ChangeListener);
>void removeChangeListener(ChangeListener);
What the information is good for?


>void runAfterTransaction(Runnable)
OK. The Runnable can be a parameter of runProtected.


>It seems this would not suffice for the purposes of bug #167098, which calls
>refreshAllIndices, i.e. versioning.indexingbridge might still be needed.
The refreshAllIndices call is questionable as it not needed for OS with native listeners.
Also the VCS exactly knows which files were changed, scheduling up to date check of everything
seems to me wrong. The VCS should schedule rescan of changed files only.
Comment 2 Jesse Glick 2012-03-01 22:02:16 UTC
(In reply to comment #1)
> JG01: We should create a single module with "IndexingBridgeProvider" on which
> versioning, ant and maven [and projectui?] will depend.
> The implementation of this interface will be provided by [parsing.api].
> Just one api or friend-api module will be added and the
> original versioning bridge will be removed.

This can work I think. I will try it.

> It does not use the threadIds to block only the changes [from] given thread.

I will document this then.

> The runProtected in public API ensures the balanced locking. 

Of course. I will check if it is possible to use this from things like Ant execution.

>> void addChangeListener(ChangeListener);
>> void removeChangeListener(ChangeListener);
>
> What the information is good for?

The above would be for use from parsing.api, in case of a semaphore-style API.

> The refreshAllIndices call is questionable as it not needed for OS with native
> listeners.
> Also the VCS exactly knows which files were changed, scheduling up to date
> check of everything
> seems to me wrong. The VCS should schedule rescan of changed files only.

Well this is beyond my area of knowledge (bug #167098) so I would leave it alone.
Comment 3 Jesse Glick 2012-03-01 23:58:33 UTC
core-main #fc07aef4b575
Comment 4 Tomas Zezula 2012-03-02 18:16:38 UTC
Yes, the IndexingManager.runProtected really deserves better documentation.
I will update it.
Comment 5 Jesse Glick 2012-03-03 01:03:36 UTC
fc07aef4b575 includes some improvements to runProtected's Javadoc, but you should review and update.
Comment 6 Quality Engineering 2012-03-03 11:32:40 UTC
Integrated into 'main-golden', will be available in build *201203030400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/fc07aef4b575
User: Jesse Glick <jglick@netbeans.org>
Log: #208213: IndexingManager.runProtected from project system operations
Comment 7 Tomas Zezula 2012-03-12 16:00:20 UTC
Thanks, the updated javadoc is good.
I've only removed the:

"Also note that this call is not reentrant."

The call is reentrant, the thread ids are kept in the List so it's safe to do
IM.runProtected(()->{IM.runProtected(()->{....});})
in the inner Callable there will be 2 same thread ids in the protectedOwners list.