Bug 177329 - RSEPersistenceManager locks the workspace and handles threads inconsistently
Summary: RSEPersistenceManager locks the workspace and handles threads inconsistently
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 1.0.1   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 2.0   Edit
Assignee: David Dykstal CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 170932
  Show dependency tree
 
Reported: 2007-03-14 09:28 EDT by Martin Oberhuber CLA
Modified: 2008-08-13 13:18 EDT (History)
4 users (show)

See Also:


Attachments
Patch making state handling synchronized (3.44 KB, patch)
2007-03-14 09:52 EDT, Martin Oberhuber CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2007-03-14 09:28:57 EDT
The RSEPersistenceManager, as it is today, is not thread-safe.

If a thread tries to load or save a profile while another load or save operation is still ongoing, the operation just returns successful=false.

Intended operation of this should either be clearly documented:
 - what is the client expected to do? 
 - Wait a little and try again later? 
 - Or expect the ongoing operation to already do what it wants? 
 - Or can't it just happen that multiple threads access the PersistenceManager
   at the same time thanks to Jobs and SchedulingRules?

Or, the RSEPersistenceManger should be fixed in a way that is more friendly for the client, e.g. use the existing RSE class "Mutex" to acquire a lock and wait until the ongoing operation completes.
Comment 1 Martin Oberhuber CLA 2007-03-14 09:52:05 EDT
Created attachment 60798 [details]
Patch making state handling synchronized

In any case, the state handling in RSEPersistenceManager is not synchronized and thus not thread-safe. Assuming that the current functionality of just returning successful=false in case of multi-thread access, this would be a bug which is fixed by attached patch (unless we can know and specify by APIdoc that no two threads may access the RSEPersistenceManager concurrently, but it doesn't seem so).

As stated in the description, the current functionality seems not optimal and it would be better to have some callback or "wait until..." functionality. Marking the bug as [api] since a potential choice to implement some "wait until..." functionality would indeed mean an API change. Such "wait until" would also be related to bug 177332 (but it could also be implemented by setting a flag or doing a callback).
Comment 2 Martin Oberhuber CLA 2007-04-05 05:18:40 EDT
The other thing is that RSEPersistenceManager currently does its task in a WorkspaceJob with a scheduling rule that avoids accessing the disk when the workspace is locked (e.g. due to an ongoing build).

In reality, the Persistence Provider should be able to decide whether workspace locking is necessary or not. A provider that does not store its data in the workspace has no need to have the workspace locked.

This workspace locking is problematic for our product, since we can have builds going on at initial Eclipse startup that do need to lock the workspace. We want to provide our own PersistenceProvider, but it is important that the PersistanceManager does not lock the workspace unnecessarily.
Comment 3 David Dykstal CLA 2007-05-10 18:33:10 EDT
Minor changes have been made to the IRSEPersistenceManager, IRSEPersistenceProvider, and RSEDOM interfaces to accomodate this request.

IRSEPersistenceManager has dropped the ability to restore a single profile, and added the ability to restore all profiles known to a particular provider. Also added is the ability to test, from a separate thread, if a restore for a particular provider is complete.

IRSEPersistenceProvider has added a "getSaveJob()" method so that the persistence provider can determine the nature and locking of the save. If there is no save job defined, then the save occurs in the thread of the persistence manager's save invocation.

RSEDOM was modified to remove the save job specification since that has been moved to the persistence provider.

The implementation objects have been updated accordingly. In addition, the PropertyFileProvider has been reorganized with the intent of providing persistence somewhere else other than in the workspace.

The patch for testing the internal state of the persistence manager in a thread safe manner has been implemented.
Comment 4 David Dykstal CLA 2007-05-11 00:03:41 EDT
More changes to IRSEPersistenceManager to add timeout values to commit and restore. Implementation now uses Mutex rather than synchronized to serialize operations. Return values from operations now indicate more status than simple failure or success. Removed exporting and importing predicates in favor of a more simple "busy" indicator.
Comment 5 Martin Oberhuber CLA 2008-08-13 13:18:24 EDT
[target cleanup] 2.0 M7 was the original target milestone for this bug