QPID-5138: Simplify file locking meachsnism and improve the implementation of prefgerences store

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1529357 13f79535-47bb-0310-9956-ffa450edef68

2 files changed, 243 insertions, 203 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProvider.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProvider.java
index f7560a0dfa..365393ee8d 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProvider.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProvider.java
@@ -23,8 +23,6 @@ package org.apache.qpid.server.model.adapter;
 
 import java.io.ByteArrayOutputStream;
 import java.io.File;
-import java.io.FileNotFoundException;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.RandomAccessFile;
 import java.lang.reflect.Type;
@@ -44,7 +42,6 @@ import java.util.UUID;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.log4j.Logger;
-import org.apache.qpid.AMQStoreException;
 import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
 import org.apache.qpid.server.model.AuthenticationProvider;
@@ -68,12 +65,9 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
     public static String PATH = "path";
     public static final String PROVIDER_TYPE = "FileSystemPreferences";
 
-    // TODO: use resolver to resolve path from
-    // '${qpid.work_dir}/preferences/${authenticationProviderName}'
     @SuppressWarnings("serial")
     private static final Map<String, Object> DEFAULTS = Collections.unmodifiableMap(new HashMap<String, Object>()
     {{
-            put(PATH, System.getProperty("user.home") + File.separator + ".qpid" + File.separator + "preferences.json");
             put(TYPE, FileSystemPreferencesProvider.class.getSimpleName());
     }});
 
@@ -95,24 +89,17 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
     private final AuthenticationProvider _authenticationProvider;
     private AtomicReference<State> _state;
 
-    private final ObjectMapper _objectMapper;
-    private final Map<String, Map<String, Object>> _preferences;
-    private File _preferencesLocation;
-    private FileLock _fileLock;
+    private FileSystemPreferencesStore _store;
 
-    protected FileSystemPreferencesProvider(UUID id, Map<String, Object> attributes, AuthenticationProvider authenticationProvider, TaskExecutor taskExecutor)
+    protected FileSystemPreferencesProvider(UUID id, Map<String, Object> attributes, AuthenticationProvider authenticationProvider,
+            TaskExecutor taskExecutor)
     {
         super(id, DEFAULTS, MapValueConverter.convert(attributes, ATTRIBUTE_TYPES), taskExecutor);
         State state = MapValueConverter.getEnumAttribute(State.class, STATE, attributes, State.INITIALISING);
         _state = new AtomicReference<State>(state);
         addParent(AuthenticationProvider.class, authenticationProvider);
         _authenticationProvider = authenticationProvider;
-        _objectMapper = new ObjectMapper();
-        _objectMapper.configure(SerializationConfig.Feature.INDENT_OUTPUT, true);
-        _objectMapper.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
-        _preferences = new TreeMap<String, Map<String, Object>>();
-        _preferencesLocation = new File(MapValueConverter.getStringAttribute(PATH, attributes));
-        _preferences.putAll(load(_objectMapper, _preferencesLocation));
+        _store = new FileSystemPreferencesStore(new File(MapValueConverter.getStringAttribute(PATH, attributes)));
     }
 
     @Override
@@ -224,8 +211,7 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
     }
 
     @Override
-    public boolean setState(State currentState, State desiredState) throws IllegalStateTransitionException,
-            AccessControlException
+    public boolean setState(State currentState, State desiredState) throws IllegalStateTransitionException, AccessControlException
     {
         State state = _state.get();
         if (desiredState == State.DELETED)
@@ -235,11 +221,11 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
             {
                 try
                 {
-                    close();
+                    _store.close();
                 }
                 finally
                 {
-                    _preferencesLocation.delete();
+                    _store.delete();
                     _authenticationProvider.setPreferencesProvider(null);
                 }
                 return true;
@@ -256,9 +242,7 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
             {
                 try
                 {
-                    getFileLock();
-                    Map<String, Map<String, Object>> preferences = load(_objectMapper, _preferencesLocation);
-                    setPreferences(preferences);
+                    _store.open();
                     return true;
                 }
                 catch (Exception e)
@@ -284,7 +268,7 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
         {
             if (state == State.INITIALISING && _state.compareAndSet(state, State.QUIESCED))
             {
-                close();
+                _store.close();
                 return true;
             }
         }
@@ -292,12 +276,12 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
         {
             if (_state.compareAndSet(state, State.STOPPED))
             {
-                close();
+                _store.close();
                 return true;
             }
             else
             {
-                throw new IllegalStateException("Cannot stop authentication preferences in state: " + state);
+                throw new IllegalStateException("Cannot stop preferences preferences in state: " + state);
             }
         }
 
@@ -307,61 +291,25 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
     @Override
     public Map<String, Object> getPreferences(String userId)
     {
-        Map<String, Object> userPreferences = null;
-        synchronized (_preferences)
-        {
-            userPreferences = _preferences.get(userId);
-        }
-        if (userPreferences != null)
-        {
-            return new HashMap<String, Object>(userPreferences);
-        }
-        return Collections.emptyMap();
+        return _store.getPreferences(userId);
     }
 
     @Override
     public Map<String, Object> setPreferences(String userId, Map<String, Object> preferences)
     {
-        Map<String, Object> userPreferences = null;
-        synchronized (_preferences)
-        {
-            userPreferences = _preferences.get(userId);
-            if (userPreferences == null)
-            {
-                userPreferences = new HashMap<String, Object>(preferences);
-                _preferences.put(userId, userPreferences);
-            }
-            else
-            {
-                userPreferences.putAll(preferences);
-            }
-            savePreferences();
-        }
-        return userPreferences;
+        return _store.setPreferences(userId, preferences);
     }
 
     @Override
     public Map<String, Object> deletePreferences(String userId)
     {
-        Map<String, Object> userPreferences = null;
-        synchronized (_preferences)
-        {
-            if (_preferences.containsKey(userId))
-            {
-                userPreferences = _preferences.remove(userId);
-                savePreferences();
-            }
-        }
-        return userPreferences;
+        return _store.deletePreferences(userId);
     }
 
     @Override
     public Set<String> listUserIDs()
     {
-        synchronized (_preferences)
-        {
-            return Collections.unmodifiableSet(_preferences.keySet());
-        }
+        return _store.listUserIDs();
     }
 
     public AuthenticationProvider getAuthenticationProvider()
@@ -377,36 +325,39 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
         validateAttributes(effectiveAttributes);
         String effectivePath = (String) effectiveAttributes.get(PATH);
         String currentPath = (String) getAttribute(PATH);
-        Map<String, Map<String, Object>> newPreferences = null;
+
         File storeFile = new File(effectivePath);
+        FileSystemPreferencesStore newStore = null;
         if (!effectivePath.equals(currentPath))
         {
             if (!storeFile.exists())
             {
                 throw new IllegalConfigurationException("Path to preferences file does not exist!");
             }
-            newPreferences = load(_objectMapper, storeFile);
+            newStore = new FileSystemPreferencesStore(storeFile);
+            newStore.open();
         }
-        super.changeAttributes(attributes);
 
-        if (newPreferences != null)
+        try
         {
-            setPreferences(newPreferences);
-            _preferencesLocation = storeFile;
-        }
+            super.changeAttributes(attributes);
 
-        // if provider was previously in ERRORED state then set its state to
-        // ACTIVE
-        _state.compareAndSet(State.ERRORED, State.ACTIVE);
-    }
-
-    private void setPreferences(Map<String, Map<String, Object>> preferences)
-    {
-        synchronized (_preferences)
+            if (newStore != null)
+            {
+                _store.close();
+                _store = newStore;
+                newStore = null;
+            }
+        }
+        finally
         {
-            _preferences.clear();
-            _preferences.putAll(preferences);
+            if (newStore != null)
+            {
+                newStore.close();
+            }
         }
+        // if provider was previously in ERRORED state then set its state to ACTIVE
+        _state.compareAndSet(State.ERRORED, State.ACTIVE);
     }
 
     private void validateAttributes(Map<String, Object> attributes)
@@ -432,161 +383,242 @@ public class FileSystemPreferencesProvider extends AbstractAdapter implements Pr
         }
     }
 
-    public File createStoreIfNotExist()
+    public void createStoreIfNotExist()
     {
-        String path = (String)getAttribute(PATH);
-        File preferencesLocation = new File(path);
-        if (!preferencesLocation.exists())
-        {
-            File parent = preferencesLocation.getParentFile();
-            if (!parent.exists() && !parent.mkdirs())
-            {
-                throw new IllegalConfigurationException("Cannot store preferences at " + path);
-            }
-            try
-            {
-                preferencesLocation.createNewFile();
-            }
-            catch (IOException e)
-            {
-                throw new IllegalConfigurationException("Cannot store preferences at " + path);
-            }
-        }
-        return preferencesLocation;
+        _store.createIfNotExist();
     }
 
-    private Map<String, Map<String, Object>> load(ObjectMapper mapper, File file)
+    public static class FileSystemPreferencesStore
     {
-        if (!file.exists() || file.length() == 0)
+        private final ObjectMapper _objectMapper;
+        private final Map<String, Map<String, Object>> _preferences;
+        private File _storeFile;
+        private FileLock _storeLock;
+        private RandomAccessFile _storeRAF;
+
+        public FileSystemPreferencesStore(File preferencesFile)
         {
-            return Collections.emptyMap();
+            _storeFile = preferencesFile;
+            _objectMapper = new ObjectMapper();
+            _objectMapper.configure(SerializationConfig.Feature.INDENT_OUTPUT, true);
+            _objectMapper.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
+            _preferences = new TreeMap<String, Map<String, Object>>();
         }
 
-        try
+        public void createIfNotExist()
         {
-            return mapper.readValue(file, new TypeReference<Map<String, Map<String, Object>>>()
+            if (!_storeFile.exists())
             {
-            });
+                File parent = _storeFile.getParentFile();
+                if (!parent.exists() && !parent.mkdirs())
+                {
+                    throw new IllegalConfigurationException("Cannot create preferences store folders");
+                }
+                try
+                {
+                    if (_storeFile.createNewFile() && !_storeFile.exists())
+                    {
+                        throw new IllegalConfigurationException("Preferences store file was not created:" + _storeFile.getAbsolutePath());
+                    }
+                }
+                catch (IOException e)
+                {
+                    throw new IllegalConfigurationException("Cannot create preferences store file");
+                }
+            }
         }
-        catch (JsonProcessingException e)
+
+        public void delete()
         {
-            throw new IllegalConfigurationException("Cannot parse json", e);
+            if (_storeFile.exists() && !_storeFile.delete())
+            {
+                LOGGER.warn("Failed to delete preferences provider file '" + _storeFile.getName() + "'");
+            }
         }
-        catch (IOException e)
+
+        public void open()
         {
-            throw new IllegalConfigurationException("Cannot read json", e);
-        }
-    }
+            if (!_storeFile.exists())
+            {
+                throw new IllegalConfigurationException("Preferences file does not exist");
+            }
 
-    private void savePreferences()
-    {
-        save(_objectMapper, _preferencesLocation, _preferences);
-    }
+            if (_storeLock != null)
+            {
+                throw new IllegalStateException("Preferences store is already opened");
+            }
+            try
+            {
+                _storeRAF = new RandomAccessFile(_storeFile, "rw");
+                FileChannel fileChannel = _storeRAF.getChannel();
+                try
+                {
+                    _storeLock = fileChannel.tryLock();
+                }
+                catch (OverlappingFileLockException e)
+                {
+                    _storeLock = null;
+                }
+                if (_storeLock == null)
+                {
+                    throw new IllegalConfigurationException("Cannot get lock on store file " + _storeFile.getName()
+                            + " is another instance running?");
+                }
+                long fileSize = fileChannel.size();
+                if (fileSize > 0)
+                {
+                    ByteBuffer buffer = ByteBuffer.allocate((int) fileSize);
+                    fileChannel.read(buffer);
+                    buffer.rewind();
+                    buffer.flip();
+                    byte[] data = buffer.array();
+                    try
+                    {
+                        Map<String, Map<String, Object>> preferencesMap = _objectMapper.readValue(data,
+                                new TypeReference<Map<String, Map<String, Object>>>()
+                                {
+                                });
+                        _preferences.putAll(preferencesMap);
+                    }
+                    catch (JsonProcessingException e)
+                    {
+                        throw new IllegalConfigurationException("Cannot parse preferences json in " + _storeFile.getName(), e);
+                    }
+                }
+            }
+            catch (IOException e)
+            {
+                throw new IllegalConfigurationException("Cannot load preferences from " + _storeFile.getName(), e);
+            }
+        }
 
-    private void save(ObjectMapper mapper, File file, Map<String, Map<String, Object>> preferences)
-    {
-        try
+        public void close()
         {
-            RandomAccessFile raf = new RandomAccessFile(file, "rw");
-            try
+            synchronized (_preferences)
             {
-                FileChannel channel = raf.getChannel();
                 try
                 {
-                    FileLock lock = null;
+                    if (_storeLock != null)
+                    {
+                        _storeLock.release();
+                    }
+                }
+                catch (IOException e)
+                {
+                    LOGGER.error("Cannot release file lock for preferences file store", e);
+                }
+                finally
+                {
+                    _storeLock = null;
                     try
                     {
-                        lock = channel.tryLock();
-                        if (lock == null)
+                        if (_storeRAF != null)
                         {
-                            throw new IllegalConfigurationException("Cannot aquire exclusive lock on preferences file for "
-                                    + getName());
+                            _storeRAF.close();
                         }
-                        ByteArrayOutputStream baos = new ByteArrayOutputStream();
-                        mapper.writeValue(baos, preferences);
-                        channel.write(ByteBuffer.wrap(baos.toByteArray()));
                     }
-                    catch (OverlappingFileLockException e)
+                    catch (IOException e)
                     {
-                        throw new IllegalConfigurationException("Cannot aquire exclusive lock on preferences file for "
-                                + getName(), e);
+                        LOGGER.error("Cannot close preferences file", e);
                     }
                     finally
                     {
-                        if (lock != null)
-                        {
-                            lock.release();
-                        }
+                        _storeRAF = null;
+                        _preferences.clear();
                     }
                 }
-                finally
-                {
-                    channel.close();
-                }
-            }
-            finally
-            {
-                raf.close();
             }
         }
-        catch (FileNotFoundException e)
-        {
-            throw new IllegalConfigurationException("Cannot find preferences file for " + getName(), e);
-        }
-        catch (IOException e)
-        {
-            throw new IllegalConfigurationException("Cannot store preferences file for " + getName(), e);
-        }
-    }
 
-    private void getFileLock() throws IOException, AMQStoreException
-    {
-        File lockFile = new File(getLockFileName());
-        lockFile.createNewFile();
-
-        FileOutputStream out = new FileOutputStream(lockFile);
-        FileChannel channel = out.getChannel();
-        try
+        public Map<String, Object> getPreferences(String userId)
         {
-            _fileLock = channel.tryLock();
+            checkStoreOpened();
+            Map<String, Object> userPreferences = null;
+            synchronized (_preferences)
+            {
+                userPreferences = _preferences.get(userId);
+            }
+            if (userPreferences != null)
+            {
+                return new HashMap<String, Object>(userPreferences);
+            }
+            return Collections.emptyMap();
         }
-        catch(OverlappingFileLockException e)
+
+        public Map<String, Object> setPreferences(String userId, Map<String, Object> preferences)
         {
-            _fileLock = null;
+            checkStoreOpened();
+            Map<String, Object> userPreferences = null;
+            synchronized (_preferences)
+            {
+                userPreferences = _preferences.get(userId);
+                if (userPreferences == null)
+                {
+                    userPreferences = new HashMap<String, Object>(preferences);
+                    _preferences.put(userId, userPreferences);
+                }
+                else
+                {
+                    userPreferences.putAll(preferences);
+                }
+                save();
+            }
+            return userPreferences;
         }
-        if(_fileLock == null)
+
+        public Map<String, Object> deletePreferences(String userId)
         {
-            throw new AMQStoreException("Cannot get lock on file " + lockFile.getAbsolutePath() + " is another instance running?");
+            checkStoreOpened();
+            Map<String, Object> userPreferences = null;
+            synchronized (_preferences)
+            {
+                if (_preferences.containsKey(userId))
+                {
+                    userPreferences = _preferences.remove(userId);
+                    save();
+                }
+            }
+            return userPreferences;
         }
-        lockFile.deleteOnExit();
-    }
-
-    private String getLockFileName()
-    {
-        return _preferencesLocation.getAbsolutePath() + ".lck";
-    }
 
-    public void close()
-    {
-        try
+        public Set<String> listUserIDs()
         {
-            releaseFileLock();
+            checkStoreOpened();
+            synchronized (_preferences)
+            {
+                return Collections.unmodifiableSet(_preferences.keySet());
+            }
         }
-        catch(IOException e)
+
+        private void save()
         {
-            LOGGER.error("Cannot close file system preferences provider", e);
+            checkStoreOpened();
+            try
+            {
+                ByteArrayOutputStream baos = new ByteArrayOutputStream();
+                _objectMapper.writeValue(baos, _preferences);
+                FileChannel channel = _storeRAF.getChannel();
+                long currentSize = channel.size();
+                channel.position(0);
+                channel.write(ByteBuffer.wrap(baos.toByteArray()));
+                if (currentSize > baos.size())
+                {
+                    channel.truncate(baos.size());
+                }
+            }
+            catch (IOException e)
+            {
+                throw new IllegalConfigurationException("Cannot store preferences", e);
+            }
         }
-        finally
+
+        private void checkStoreOpened()
         {
-            new File(getLockFileName()).delete();
-            _fileLock = null;
-            _preferences.clear();
+            if (_storeLock == null)
+            {
+                throw new IllegalStateException("Preferences store is not opened");
+            }
         }
-    }
 
-    private void releaseFileLock() throws IOException
-    {
-        _fileLock.release();
-        _fileLock.channel().close();
     }
 }
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProviderTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProviderTest.java
index 0eab93541f..aebea294d8 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProviderTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileSystemPreferencesProviderTest.java
@@ -69,6 +69,10 @@ public class FileSystemPreferencesProviderTest extends QpidTestCase
     {
         try
         {
+            if (_preferencesProvider != null)
+            {
+                _preferencesProvider.setDesiredState(_preferencesProvider.getActualState(), State.DELETED);
+            }
             BrokerTestHelper.tearDown();
             _preferencesFile.delete();
         }
@@ -86,7 +90,7 @@ public class FileSystemPreferencesProviderTest extends QpidTestCase
 
     public void testConstructionWithNonExistingFile()
     {
-        File nonExistingFile = new File(TMP_FOLDER, "preferences-" + UUID.randomUUID() + ".json");
+        File nonExistingFile = new File(TMP_FOLDER, "preferences-" + getTestName() + ".json");
         assertFalse("Preferences file exists", nonExistingFile.exists());
         try
         {
@@ -105,7 +109,7 @@ public class FileSystemPreferencesProviderTest extends QpidTestCase
 
     public void testConstructionWithEmptyFile() throws Exception
     {
-        File emptyPrefsFile = new File(TMP_FOLDER, "preferences-" + UUID.randomUUID() + ".json");
+        File emptyPrefsFile = new File(TMP_FOLDER, "preferences-" + getTestName() + ".json");
         emptyPrefsFile.createNewFile();
         assertTrue("Preferences file does notexists", emptyPrefsFile.exists());
         try
@@ -188,9 +192,11 @@ public class FileSystemPreferencesProviderTest extends QpidTestCase
         newPreferences.put("pref5", pref5);
 
         _preferencesProvider.setPreferences(_user1, newPreferences);
+        _preferencesProvider.setDesiredState(State.ACTIVE, State.STOPPED);
 
-        FileSystemPreferencesProvider newProvider = createPreferencesProvider();
-        Map<String, Object> preferences1 = newProvider.getPreferences(_user1);
+        _preferencesProvider = createPreferencesProvider();
+        _preferencesProvider.setDesiredState(State.INITIALISING, State.ACTIVE);
+        Map<String, Object> preferences1 = _preferencesProvider.getPreferences(_user1);
         assertNotNull("Preferences should not be null for user 1", preferences1);
         assertEquals("Unexpected preference 1 for user 1", "pref1User1Value", preferences1.get("pref1"));
         assertEquals("Unexpected preference 2 for user 1", false, preferences1.get("pref2"));
@@ -199,11 +205,11 @@ public class FileSystemPreferencesProviderTest extends QpidTestCase
         assertNotNull("Unexpected preference 5 for user 1", preferences1.get("pref5"));
         assertEquals("Unexpected preference 5 for user 1", pref5, preferences1.get("pref5"));
 
-        Map<String, Object> preferences2 = newProvider.getPreferences(_user2);
+        Map<String, Object> preferences2 = _preferencesProvider.getPreferences(_user2);
         assertUser2Preferences(preferences2);
 
         String user3 = "user3";
-        Map<String, Object> preferences3 = newProvider.getPreferences(user3);
+        Map<String, Object> preferences3 = _preferencesProvider.getPreferences(user3);
         assertTrue("No preference found for user3", preferences3.isEmpty());
     }
 
@@ -213,16 +219,18 @@ public class FileSystemPreferencesProviderTest extends QpidTestCase
         _preferencesProvider.setDesiredState(State.INITIALISING, State.ACTIVE);
 
         _preferencesProvider.deletePreferences(_user1);
+        _preferencesProvider.setDesiredState(State.ACTIVE, State.STOPPED);
 
-        FileSystemPreferencesProvider newProvider = createPreferencesProvider();
-        Map<String, Object> preferences1 = newProvider.getPreferences(_user1);
+        _preferencesProvider = createPreferencesProvider();
+        _preferencesProvider.setDesiredState(State.INITIALISING, State.ACTIVE);
+        Map<String, Object> preferences1 = _preferencesProvider.getPreferences(_user1);
         assertTrue("Preferences should not be set for user 1", preferences1.isEmpty());
 
-        Map<String, Object> preferences2 = newProvider.getPreferences(_user2);
+        Map<String, Object> preferences2 = _preferencesProvider.getPreferences(_user2);
         assertUser2Preferences(preferences2);
 
         String user3 = "user3";
-        Map<String, Object> preferences3 = newProvider.getPreferences(user3);
+        Map<String, Object> preferences3 = _preferencesProvider.getPreferences(user3);
         assertTrue("No preference found for user3", preferences3.isEmpty());
     }