Bug 251504 - [index] Wrong indexes may be used while performing a search request
Summary: [index] Wrong indexes may be used while performing a search request
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.5 M4   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 241592 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-10-21 04:38 EDT by Frederic Fusier CLA
Modified: 2020-08-27 10:47 EDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (8.22 KB, patch)
2008-11-24 05:29 EST, Frederic Fusier CLA
no flags Details | Diff
Real proposed patch (4.05 KB, patch)
2008-11-24 05:31 EST, Frederic Fusier CLA
no flags Details | Diff
Final? proposed patch (4.05 KB, patch)
2008-11-24 05:38 EST, Frederic Fusier CLA
no flags Details | Diff
Last proposed patch (4.51 KB, patch)
2008-11-24 10:56 EST, Frederic Fusier CLA
no flags Details | Diff
Last patch improvement (1.85 KB, patch)
2008-11-24 12:14 EST, Frederic Fusier CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frederic Fusier CLA 2008-10-21 04:38:55 EDT
With fix for bug 12044, attached sources of a jar file are indexed using a low priority. This low priority means that as soon as a search request is done, it will be executed immediately even if the indexing is not finished as soon as it does not need such indexes.

The problem is that the PatterSearchJob ran for the request stores all the indexes in a local array (see the method execute(IProgressMonitor)) and, at the same time, the indexing of the attached sources can modify the index files (i.e. call IndexManager.saveIndex(Index)). So, in this case, there would be a high probability that indexes in the array do not longer match the indexes of the IndexManager, hence got unpredictable results while reading them (usually an OOME)...

This is shown by the patch for bug 12044:
https://bugs.eclipse.org/bugs/attachment.cgi?id=115273
but it's a potential issue for all callers of IndexManager.getIndexes(IPath[], IProgressMonitor) which may stores the array locally, hence needs a general fixes to prevent such kind of troubles...
Comment 1 Frederic Fusier CLA 2008-11-24 05:29:58 EST
Created attachment 118573 [details]
Proposed patch

Patch extracted from last bug 12044 patch and improved a little bit.

With this patch, the index is not recreated while indexing a jar file, but reset instead. This avoid a running request to lose the index pointer if a re-indexing occurs concurrently (see PatternSearchJob.execute(IProgressMonitor) method)...

Note that the index will be recreated if none is found while resetting it...
Comment 2 Frederic Fusier CLA 2008-11-24 05:31:45 EST
Created attachment 118574 [details]
Real proposed patch

Previous patch included temporary changes DiskIndex which I think are not finalized yet...
Comment 3 Frederic Fusier CLA 2008-11-24 05:38:34 EST
Created attachment 118575 [details]
Final? proposed patch

I got some troubles this morning with patches...
Previous one has a compiler error!
Comment 4 Frederic Fusier CLA 2008-11-24 07:19:12 EST
Jerome, Kent, could you have a look on the patch and let me know your mind about it?
TIA
Comment 5 Jerome Lanneluc CLA 2008-11-24 10:42:15 EST
(In reply to comment #3)
It looks like the index file is reused when reset (actually only the header is reused). I'm not sure what the content of the header is, but it might be safer to do as before: delete the file and recreate it from scratch. 
Comment 6 Frederic Fusier CLA 2008-11-24 10:55:47 EST
(In reply to comment #5)
> (In reply to comment #3)
> It looks like the index file is reused when reset (actually only the header is
> reused). I'm not sure what the content of the header is, but it might be safer
> to do as before: delete the file and recreate it from scratch. 
> 
Good point. I thought it could be a possible optimization not delete/recreate the index file, but then we would keep categories tables offset in the header which make its contents invalid. It seems that the DiskIndex.initialize(boolean) can be used only for to clone DiskIndex objects when the argument is true...
Comment 7 Frederic Fusier CLA 2008-11-24 10:56:35 EST
Created attachment 118601 [details]
Last proposed patch
Comment 8 Frederic Fusier CLA 2008-11-24 10:59:29 EST
Released for 3.5M4 in HEAD stream.

Unfortunately it was not possible to write test for the fix of this bug.
I'm afraid that only code review can be done to verify it...
Comment 9 Frederic Fusier CLA 2008-11-24 12:14:30 EST
Created attachment 118611 [details]
Last patch improvement

Additional change to always create a new index file while resetting the Index.
Thanks Jerome :-)
Comment 10 Kent Johnson CLA 2008-12-09 13:58:50 EST
Verified for 3.5M4 using I20081209-0100
Comment 11 Frederic Fusier CLA 2009-03-06 03:25:18 EST
*** Bug 241592 has been marked as a duplicate of this bug. ***