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 168237 - Indices may get out of date
Summary: Indices may get out of date
Status: RESOLVED FIXED
Alias: None
Product: editor
Classification: Unclassified
Component: Parsing & Indexing (show other bugs)
Version: 6.x
Hardware: All All
: P2 blocker (vote)
Assignee: Vitezslav Stejskal
URL: http://wiki.netbeans.org/OutOfDateInd...
Keywords:
: 135894 167537 168432 (view as bug list)
Depends on: 33162 170727
Blocks:
  Show dependency tree
 
Reported: 2009-07-07 16:04 UTC by Jaroslav Tulach
Modified: 2009-09-11 21:37 UTC (History)
15 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Test simulating external changes (17.82 KB, patch)
2009-07-07 16:04 UTC, Jaroslav Tulach
Details | Diff
A prototype patch keeping FOs for active source roots. (9.19 KB, patch)
2009-08-19 13:39 UTC, Jan Lahoda
Details | Diff
Updated patch - handles file/folder creation/deletion more correctly. (10.06 KB, patch)
2009-08-20 17:07 UTC, Jan Lahoda
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2009-07-07 16:04:18 UTC
The parsing infrastructure does not work well with respect to external modifications. When a file under a source root is
modified externally, its index data may never be refreshed. This is often the case for project types that use tree as a
logical view (as that may mean there are no FileObject for some parts of the tree). Java's logical view is in better
position, as it holds all FileObjects of packages. 

As soon as this situation happens, there is no easy workaround (except restarting or closing and reopening project) as
there is no way user could trigger up to date check.

I will attach test that mimics what happens. It it a copy of already existing RepositoryUpdateTest, just instead of using
FileSystem API it directly modifies the files via java.io.File and then simulates "refresh gesture" - right now our only
refresh gesture is focusGained of NetBeans main window which triggers FileUtil.refreshAll(). The test obviously fails.

I do not want to spekulate about proper fix of this problem (yet I am CCing Jirka S. as this may be filesystem related
problem), but I consider this serious enough flaw to deserve attention.
Comment 1 Jaroslav Tulach 2009-07-07 16:04:54 UTC
Created attachment 84454 [details]
Test simulating external changes
Comment 2 David Strupl 2009-07-08 08:47:21 UTC
*** Issue 167537 has been marked as a duplicate of this issue. ***
Comment 3 Vitezslav Stejskal 2009-07-29 13:58:32 UTC
In general, this has always been a problem in Netbeans. And as it seems there is no obvious and correct solution. Let's
see if we can somehow fix/workaround this in 6.8, but if not we may just waive it.
Comment 4 Jaroslav Tulach 2009-08-03 15:46:47 UTC
I am not sure if QE knew that this is the "designed" behaviour. I definitely did not. Now, when we know, we shall not 
treat that lightly. That is at least my opinion.

I do not want to propose any solution, but I guess that even less elegant or compromising solution is better than 
waiver.
Comment 5 Dusan Balek 2009-08-06 08:40:28 UTC
*** Issue 135894 has been marked as a duplicate of this issue. ***
Comment 6 Vitezslav Stejskal 2009-08-18 09:49:49 UTC
The problem with indices getting out of sync with the underlying filesystem is not the only problem of this type in the
IDE. All these problems stem from the fact that Netbeans filesystems do not reflect OS filesystem - that is that changes
done to files on the OS level are not immediately propagated to Netbeans filesystems. Instead Netbeans filesystems poll
the state of (some) files when a user returns to the IDE. There is a long time issue #26230 requesting better
integration between Netbeans filesystems and OS filesystem events. AFAIK similar integration is going to be added to JDK7.

IMO this is the preferred solution for this issue as well.
Comment 7 Jiri Skrivanek 2009-08-18 10:39:48 UTC
Yes, JDK7 already contains such support. Filesystems could use it. But there are no plans for backport to JDK6. So, this
issue should be solved for JDK6 a different way. I know of suggestion to add implicit refresh action which would go
through selected file tree and refresh its content. I remember long discussion about it and I don't know why it was
finally rejected but IMO it is acceptable solution.
Comment 8 Jaroslav Tulach 2009-08-18 10:53:30 UTC
Issue 26230 is not a blocker for fixing this issue for two reasons:
1. if you hold all FileObjects you want to observe by yourself, you will get the notifications even now
2. there is a simple alternative fix - just add some user gesture (action in menu, for example) to synchronize the 
indices.
Due to this I suggest to remove dependency on issue 26230 and/or not consider successful fix of issue 26230 a blocker 
for fixing issue 168237.
Comment 9 Jan Lahoda 2009-08-18 12:11:21 UTC
Re jtulach's 1.: as discussed offline, this is not a feasible solution. Filesystems do refresh on each Window activated
event, and if we held all FOs, the Filesystems would effectively do equivalent of the indexing's initial timestamp check
on each Window activated event. IMO, this is totally unacceptable.
Re jtulach's 2: technically speaking, this would not fix the issue - the test above would still fail. Note that for
ant+java, the timestamp check is done on each Clean&Build. Also, the only two safe variants (known to me) of this action
are "refresh this source root including its dependencies" and "refresh all". Variants like "refresh this source root
without dependencies" and "refresh this folder without dependencies" are not safe (may lead to persistently broken caches).
Comment 10 Vitezslav Stejskal 2009-08-18 14:00:19 UTC
Re #1 - As jlahoda replied we have serious reasons to believe that this solution while technically correct would not
work well in real life situations. Additionally to what jlahoda mentioned keeping all FOs traversed during initial scan
would considerably increase memory footprint of the IDE. We know that there are projects with 50k and more files (eg.
ACE+TAO, many PHP projects, etc). Also remember that indexing traverses project dependencies recursively and may access
files in projects that are not opened, but are required by some of the opened projects. It's not unusual that there are
hundreds of source roots scanned by the IDE. The total number of FileObjects under them can be huge.

We consider this approach only a workaround for problems caused by the lack of integration between Nb filesystems and OS
filesystem as described in issue #26230.

re #2 - IMO the 'Refresh' action only moves the problem from the IDE to a user. We used to have 'Refresh' action prior
Netbeasn 4.0 when it was removed and the automated refresh mechanism was implemented. Although I can't find any
architectural documents that would explain this change and summarize reasons that led us to this decision I believe that
there were good reasons for this change. This proposal goes backwards and would get us to the pre-4.0 state. I
personally would rather see us to fix #26230 and make Nb filesystem events reliable (and remove the automatic fs
refresh) rather then reintroduce the 'Refresh' action again.
Comment 11 Jan Lahoda 2009-08-19 13:38:08 UTC
I am going to attach a prototype patch that allows to keep the FileObject for folders (or folder and files) for all
folders/files in the active source roots.

The patch does nothing by default. The following two option change the behavior:
-J-Dorg.netbeans.modules.parsing.impl.indexing.FileObjectKeeper.kind=folders == keep FOs for folders
-J-Dorg.netbeans.modules.parsing.impl.indexing.FileObjectKeeper.kind=all == keep all FOs (folders+files)

What I think that needs to be done:
-the behavior of the patch needs to be checked from the performance point of view
-in masterfs, at least the following should be done:
--exact behavior of FileChangeEvents w.r.t. external changes need to be specified and documented as a contract
--the FS refresh needs to run under atomicAction+#170544.
Comment 12 Jan Lahoda 2009-08-19 13:39:21 UTC
Created attachment 86451 [details]
A prototype patch keeping FOs for active source roots.
Comment 13 Jan Lahoda 2009-08-20 17:07:01 UTC
Created attachment 86507 [details]
Updated patch - handles file/folder creation/deletion more correctly.
Comment 14 Jesse Glick 2009-08-20 19:45:18 UTC
I tried the patch with kind=all, on a dev build (Ubuntu, JDK 6). My test case was SQE, a midsize module suite (213
*.java in 54 modules), using a binary platform with a source association (release67). I set "Dependencies in Java
Tasklist" to "Enabled". What works:

1. Edit a project file in Emacs, switch to NB, project marked modified.

2. Edit release67/openide.windows/.../TopComponent.java to be non-public, switch to NB, several modules marked with
error badges. Restore TC.java, switch to NB again, badges disappear.

3. Edit SQEManager.getDefault to be non-public, switch. Calling modules marked w/ errors. Fix, switch, errors go away.

Upon switching back to the IDE, if there are in fact external changes, you get a few seconds of high CPU activity as
projects are rescanned. If there are no external changes, there is a blip of high CPU - maybe 1sec - visible in the
process monitor, but not enough to turn the laptop fan on. So for this size project, the fully accurate mode is quite
reasonable.

Approximate memory usage after startup and initial scan:

no flag -> 100/123; after GC -> 43/148

kind=all -> 86/121; after GC -> 47/151

The default value for "Dependencies in Java Tasklist" ("Enabled within a source root" IIRC) does not work well. You get
modification badges when a file in an open project was changed, but error badges do not update until you browse to
callers, at which point "Incorrect error badge" warnings are printed on console and things refresh.

kind=folders does not work at all; the IDE does not notice that the files changed externally. It is as if the patch is
not active. I suppose that is due to issue #170727.

Based on this preliminary experience I would suggest adding a new option for "Dependencies in Java Tasklist" that would
act like the current "Enabled" plus kind=all. Label perhaps "Enabled including external edits". People could try using
this most accurate mode and see if performance is tolerable.

In the longer term, even if we cannot integrate native file notifications (as in issue #26230), I would at least suggest
a more efficient and straightforward way to poll for external changes than keeping a tree of FileObject's, which is a
lot of object-oriented trash that may be too much for people with large projects. In issue #33162 I requested mode #3b
(presupposing #2=yes, #3=yes; see my desc17, desc35, desc40, desc44, desc58 for elaboration) which was not implemented
when that issue was marked FIXED. This patch is in a way an inefficient implementation of #3b (in a client module rather
than FS API itself). The more scalable design, not requiring FileObject, would be to receive a list of File roots which
ought to be listened to, create a compact data structure representing the last observed disk state (e.g.
SortedMap<String,Long> of relpath -> lastmod), and when FU.refreshAll is called do a simple File traversal and note any
changes; finally fire an event with all the changes using a listener interface TBD.
Comment 15 Jaroslav Tulach 2009-08-21 16:34:22 UTC
I have just measured # of FileObjects in memory when $nball/editor module is open. Originally it used to be 900, now, 
with Jan's patch and patch for issue 170727 it was 2500. The refresh time after switch to the IDE was sufficiently 
fast.
Comment 16 Jaroslav Tulach 2009-08-21 17:02:56 UTC
Working with PHP mediawiki project locally is OK. When the same project is mounted over sshfs, the "scan for external 
modifications" can take minutes (but I should probably mention that the parsing API initial scan never finished in 
this setup either).
Comment 17 Vitezslav Stejskal 2009-08-24 12:23:03 UTC
Re. measurements - I don't mean to be a hassle, but would it be possible to publish the actual numbers, please, rather
than saying that things are 'sufficiently fast' or 'OK'? It might also be useful to know what hardware/OS you are
testing this on.

Re. "Enabled including external edits" - This sounds like a good idea to me. Changing these options should probably
force full rescan to make sure that the indices are in a good shape. Our current intention is to set 'Enabled' as the
default value after #170544 and #134805 are fixed. I assume that most people would switch to 'Enabled including external
edits' after discovering inconsistencies in their indices (eg. wrong error badges and similar), therefore changing this
option should also make an attempt to fix the indices.

One problem that I can see is that this specific option is not exactly only for java. Detecting external changes will
affect all indices for all languages and also eg file index. Therefore having this under Editor -> Tasklist -> Java is
IMO inappropriate. On the other hand detecting compilation errors across all projects, within a project or within a
single source root is solely java specific and these options belong to Editor -> Tasklist -> Java.
Comment 18 Vitezslav Stejskal 2009-08-24 12:23:10 UTC
Re. measurements - I don't mean to be a hassle, but would it be possible to publish the actual numbers, please, rather
than saying that things are 'sufficiently fast' or 'OK'? It might also be useful to know what hardware/OS you are
testing this on.

Re. "Enabled including external edits" - This sounds like a good idea to me. Changing these options should probably
force full rescan to make sure that the indices are in a good shape. Our current intention is to set 'Enabled' as the
default value after #170544 and #134805 are fixed. I assume that most people would switch to 'Enabled including external
edits' after discovering inconsistencies in their indices (eg. wrong error badges and similar), therefore changing this
option should also make an attempt to fix the indices.

One problem that I can see is that this specific option is not exactly only for java. Detecting external changes will
affect all indices for all languages and also eg file index. Therefore having this under Editor -> Tasklist -> Java is
IMO inappropriate. On the other hand detecting compilation errors across all projects, within a project or within a
single source root is solely java specific and these options belong to Editor -> Tasklist -> Java.
Comment 19 Jaroslav Tulach 2009-08-24 12:51:00 UTC
Better to keep result of our discussions in a wikipage: http://wiki.netbeans.org/OutOfDateIndexes
Comment 20 Jan Lahoda 2009-08-24 14:30:49 UTC
FYI: this options enables some debugging info about the refresh:
-J-Dorg.netbeans.modules.masterfs.REFRESH.level=0
Comment 21 Jaroslav Tulach 2009-08-31 14:36:27 UTC
Today's decision of release coordinator was to not use the improved automatical refresh. The next option being 
considered is to provide global refresh action (if HIE) can agree with that. Reassigning to Víťa as the one who drives 
the refresh action effort.
Comment 22 Jaroslav Tulach 2009-08-31 16:32:58 UTC
No changes in filesystems API are prerequisites for implementation of global refresh action. Removing dependencies on 
filesystem related issues.
Comment 23 Tomas Zezula 2009-09-04 13:04:09 UTC
*** Issue 168432 has been marked as a duplicate of this issue. ***
Comment 24 Vitezslav Stejskal 2009-09-07 17:41:29 UTC
Sources -> 'Scan for external changes' action added:
http://hg.netbeans.org/jet-main/rev/91150cff1620
http://hg.netbeans.org/jet-main/rev/314a9ac78fb0
Comment 25 Jaroslav Tulach 2009-09-08 07:15:49 UTC
If the action can change its isEnabled return value, shall it not also deliver property change events?
Comment 26 Quality Engineering 2009-09-08 12:27:18 UTC
Integrated into 'main-golden', will be available in build *200909080527* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/91150cff1620
User: Vita Stejskal <vstejskal@netbeans.org>
Log: #168237: 'Scan for external changes' action
Comment 27 Vitezslav Stejskal 2009-09-08 13:53:44 UTC
"If the action can change its isEnabled return value, shall it not also deliver property change events?" - Right, I
think it should.
Comment 28 Vitezslav Stejskal 2009-09-10 06:52:58 UTC
http://hg.netbeans.org/jet-main/rev/2e3e2130a9f7
Comment 29 Quality Engineering 2009-09-11 21:37:22 UTC
Integrated into 'main-golden', will be available in build *200909111401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/2e3e2130a9f7
User: Vita Stejskal <vstejskal@netbeans.org>
Log: #168237: (cont'd) fixing typo and moving SFEC action regostration to parsing.api's layer