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 12853 - DataObject registration in the DataObjectPool is done on partially constructed DataObjects
Summary: DataObject registration in the DataObjectPool is done on partially constructe...
Status: CLOSED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: -- Other -- (show other bugs)
Version: 3.x
Hardware: Sun Solaris
: P2 blocker (vote)
Assignee: Jaroslav Tulach
URL:
Keywords:
Depends on:
Blocks: 20022
  Show dependency tree
 
Reported: 2001-06-14 17:34 UTC by Chris Webster
Modified: 2008-12-22 21:55 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
diff against version 1.45 of MultiDataObject.java (903 bytes, patch)
2001-06-15 19:54 UTC, Joseph Cortopassi
Details | Diff
Diff against version 1.33.2.3.6.1 of DataObjectPool.java (5.46 KB, patch)
2001-06-15 19:56 UTC, Joseph Cortopassi
Details | Diff
Patch with sources and classes that runs in release32 (34.09 KB, application/octet-stream)
2001-06-27 07:19 UTC, Jaroslav Tulach
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Webster 2001-06-14 17:34:11 UTC
While developing regression tests, a condition which is not thread safe
under multiple CPU machines has been encountered many times in regression
testing. The general problem is that creation of a DataObject is not performed
synchronously, which may result in a DataObject being returned which is not
initialized. Specifically, there are two problems during DataObject creation: 

1) the DataObjectPool registration happens in DataObject's constructor,
2) once this call to register is done other threads can (and do) find this 
partially constructed object in the DataObjectPool
3) we consistently see test failures because of this on MP machines. 

The following example illustrates one stack trace to obtain these results.

Thread 1 -- CPU 1
DataObject.find
  -> getLoaderPool().findDataObject()
  -> DataLoader.findDataObject()
  -> DataObjectPool.callHandleFind()
  -> DataLoader.handleFindDataObject()
Assume MultiFileLoader          
  -> MultiFileLoader.createMultiObject()
  -> MultiDataObjectInstance().super()
  // at this point the MDOInstance constructor has not completed
  -> MultiDataObject.MultiDataObject.super
  -> DataObject.DataObject
  -> DataObjectPool.register()
  -> map.put
  -> loses control
                                                                

Thread 2 starts
DataObject.find
        -> DataObjectPool.find
        Can return partially intialized DataObject. 
        
Thread 2 may then start using a partially initialized DataObject.

Investigating this issue also highlighted a performance problem in the
loading process. As you can see from the pseudo stack trace from Thread
1, a find may cause unnecessary garbage collection. The final test to
determine if the object exists is performed in the constructor, where
the memory has already been allocated. Another problem, is that
exceptions, namely DataObjectExistsException are being used to handle
control. A solution should eliminate the unnecessary object creation,
possibly by separating the creation and find stage and moving to a
synchronized block.

Other potential problems:
DataObjectPool.find accesses does not access toNotify in a synchronized block.
This can lead to threads running on other CPU's not seeing this value was set in
the register block. Solution: changed DataObjectPool.find to

public synchronized DataObject find(FileObject fo) {
  if (toNotify != null) {
      return null;
  }
  
  Item doh = (Item) map.get(fo);
  return doh == null? null: doh.getDataObjectOrNull();
}
Comment 1 Jaroslav Tulach 2001-06-15 09:56:07 UTC
1. Did the suggested change in synchronization helped you? Should we apply it?
2. "The final test to determine if the object exists is performed in the
constructor" - the test is performed before the constructor, because the method
DataObjectPool.POOL.register is static. As far as I know the object is not
allocated yet.
3. "exceptions, namely DataObjectExistsException are being used" - true. But it
is hard to do anything with it and keep backward compatibility.


I'll apply the change in synchronization in main trunk. Let me know whether and
in which branches it should be applied too.
Comment 2 Joseph Cortopassi 2001-06-15 19:54:23 UTC
Created attachment 1600 [details]
diff against version 1.45 of MultiDataObject.java
Comment 3 Joseph Cortopassi 2001-06-15 19:56:45 UTC
Created attachment 1601 [details]
Diff against version 1.33.2.3.6.1 of DataObjectPool.java
Comment 4 Joseph Cortopassi 2001-06-15 19:58:19 UTC
Synchronizing the access to toNotify in DataObjectPool.find does not fix the 
problem. It is really just a side issue. The main problem is that since the 
call to DataObjectPool.register is done in DataObject's constructor, the 
registration finishes before the data object's subclass constructors have run 
and thus a partially constructed data object is in the pool's table and 
available for another thread to find and use.

It seems like any potential fix can not complete the registration until the 
newly created data object is fully constructed. Since we probably can't change 
the protocol used to create data objects, it is not clear how to do this unless 
you know every place that creates a new data object.

We did play with a partial fix that seemed to work for out test. In 
DataObjectPool.register we mark the Item for the new data object as not 
initialized. Other places in DataObjectPool that do "get" on the map 
synchronize on the found Item and if it is not initialized, then they wait
on the found Item. callHandleFind marks the Item for the found data object
as initialized and then does a notifyAll on the found Item. Thus the other 
places in DataObjectPool won't ever return an uninitialized data object.

Of course this requires other places that create a data object to somehow get 
the data object's Item marked as initialized. We changed 
MultiDataObject.handleCreateFromTemplate to call a new method in DataObjectPool 
to deal with this, but this would have to been done at any place a DataObject 
is created, and I have no idea whether that is pratical (for example are data 
objects deserialized?). I'm including the changed files as an attachment but
I need to say again, I don't consider this a fix since I have no idea all the
ways and places a DataObject may be created. Hopefully someone who understands 
all the ways DataObject can be created can figure out how to get something like 
this working.
Comment 5 Chris Webster 2001-06-15 21:08:35 UTC
Additional comments and changes posted,problem still remains after fix.
Comment 6 Jaroslav Tulach 2001-06-19 08:28:45 UTC
I do not want to add new field into DataObjectPool.Item to save a bit of memory.
I'll try to enhance the toNotify mechanism which was originaly designed to
prevent the problems described in this bug, but does not seem to be correct.
Comment 7 Jaroslav Tulach 2001-06-19 10:20:58 UTC
Fixed. DataObjectPool rev. 1.41 and DataLoaderPool rev. 1.67

I've keep the toNotify mechanism, but to new approach is to wait at least 200ms
before cleaning the toNotify HashSet. More details:

If a data object is created by DataLoaderPool.findDataObject or by
DataObject.find, immediatelly after the object is created a
DataObjectPool.notifyCreated (dataObject) method is called, object is removed
from toNotify and if toNotify is empty, it is set to null.

(by toNotify == null, one can use the DataObjectPool.find (FileObject) that
caused the problems that lead to this bug).

If a data object is created in some other way - for example by invoking its
constructor directly. It is added into toNotify set and a task is started to
later notify its creation. After 200ms of no changes in toNotify set, all its
objects are expected to finish their constructors and are notified. 

The second case is just fallback case for strange situations when one is not
using DataObject.find, but should guarantee that creation of all objects is
notified to listeners sometimes.
Comment 8 Chris Webster 2001-06-19 21:59:38 UTC
Can this fix be added to 3.2.1
Comment 9 Jan Zajicek 2001-06-20 14:54:32 UTC
Chris, can you verify this issue? Thanks.

If you will this have in 3.2.1, add keyword 3.2.1_CANDIDATE, the issue have to
be verified and reviewed.
Comment 10 Chris Webster 2001-06-25 17:54:13 UTC
Would you verification be valid by using only the new versions of
MultiDataObject and DataObjectPool with the rest of the source from the
pilsen_fcs branch?   
Comment 11 Jan Zajicek 2001-06-26 08:42:09 UTC
The verification will be valid when using DataObjectPool rev. 1.41 and
DataLoaderPool rev. 1.67.
Comment 12 Chris Webster 2001-06-26 19:33:00 UTC
When I update DataObjectPool and DataLoaderPool
cvs -r update -r 1.41 DataObjectPool.java
cvs -r update -r 1.67 DataLoaderPool.java 
the following compilation errors result when compiling against the pilsen_fcs
branch: It seems as though other files have changed as well since the pilsen_fcs
branch. Any ideas

compile-regular:
    [javac] Compiling 2 source files to
/bling1/home/cwebster/cvs/nb_all/openide/src
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:472:
cannot resolve symbol
    [javac] symbol  : constructor UniFileLoader  (java.lang.String)
    [javac] location: class org.openide.loaders.UniFileLoader
    [javac]             super ("org.openide.loaders.DataFolder"); 
    [javac]             ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:526:
cannot resolve symbol
    [javac] symbol  : class Folder  
    [javac] location: class org.openide.loaders.FileEntry
    [javac]             return new FileEntry.Folder(obj, primaryFile);
    [javac]                                 ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:530:
incompatible types
    [javac] found   : org.openide.loaders.DataFolder
    [javac] required: org.openide.loaders.MultiDataObject
    [javac]             return new DataFolder (primaryFile);
    [javac]                    ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:551:
cannot resolve symbol
    [javac] symbol  : constructor UniFileLoader  (java.lang.String)
    [javac] location: class org.openide.loaders.UniFileLoader
    [javac]             super ("org.openide.loaders.InstanceDataObject"); 
    [javac]             ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:646:
cannot resolve symbol
    [javac] symbol  : constructor MultiFileLoader  (java.lang.String)
    [javac] location: class org.openide.loaders.MultiFileLoader
    [javac]             super ("org.openide.loaders.DefaultDataObject"); 
    [javac]             ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:749:
cannot resolve symbol
    [javac] symbol  : constructor UniFileLoader  (java.lang.String)
    [javac] location: class org.openide.loaders.UniFileLoader
    [javac]             super ("org.openide.loaders.DataShadow");
    [javac]             ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:797:
incompatible types
    [javac] found   : org.openide.loaders.DataShadow
    [javac] required: org.openide.loaders.MultiDataObject
    [javac]                 if (d != null) return new DataShadow (primaryFile,
d, this);
    [javac]                                       ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataLoaderPool.java:802:
incompatible types
    [javac] found   : org.openide.loaders.BrokenDataShadow
    [javac] required: org.openide.loaders.MultiDataObject
    [javac]             return new BrokenDataShadow (primaryFile, this);
    [javac]                    ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataObjectPool.java:119:
cannot resolve symbol
    [javac] symbol  : method changedDataSystem 
(org.openide.filesystems.FileObject)
    [javac] location: class org.openide.loaders.FolderList
    [javac]                             FolderList.changedDataSystem (file);
    [javac]                                       ^
    [javac]
/bling1/home/cwebster/cvs/nb_all/openide/src/org/openide/loaders/DataObjectPool.java:630:
cannot resolve symbol
    [javac] symbol  : method changedDataSystem 
(org.openide.filesystems.FileObject)
    [javac] location: class org.openide.loaders.FolderList
    [javac]                 FolderList.changedDataSystem (df.getPrimaryFile ());
    [javac]                           ^
    [javac] 10 errors

BUILD FAILED

/bling1/home/cwebster/cvs/nb_all/openide/build.xml:44: Compile failed, messages
should have been provided.

Comment 13 Jaroslav Tulach 2001-06-27 07:17:56 UTC
You are right, Chris, it is not as easy. There were more changes in the main
trunk lately.
Comment 14 Jaroslav Tulach 2001-06-27 07:19:19 UTC
Created attachment 1743 [details]
Patch with sources and classes that runs in release32
Comment 15 Jaroslav Tulach 2001-06-27 07:21:35 UTC
Please evaluate and add keyword for release32 if desired. Otherwise seems to be
fixed.
Comment 16 Chris Webster 2001-06-29 22:16:43 UTC
The regression test detecting the problem was run with the new change, but the
change did not resolve the problem. Therefore, I do not recommend integration
into NB 3.2.1
Comment 17 Quality Engineering 2003-07-01 15:57:24 UTC
Resolved for 3.4.x or earlier, no new info since then -> verified
Comment 18 Quality Engineering 2003-07-01 16:17:37 UTC
Resolved for 3.4.x or earlier, no new info since then -> closing.