From 49731f586268c2f0c97ba2d4b0d75699f81d60f6 Mon Sep 17 00:00:00 2001 From: Robert von Burg Date: Wed, 4 Sep 2019 12:38:59 +0200 Subject: [PATCH] [Major] Refactored locking xmlpers to always first lock parent, and unlock in TX --- .../java/li/strolch/xmlpers/api/FileDao.java | 101 ++++----- .../java/li/strolch/xmlpers/api/FileIo.java | 35 +-- .../li/strolch/xmlpers/api/MetadataDao.java | 88 +++----- .../li/strolch/xmlpers/api/ObjectDao.java | 207 ++++++++---------- .../xmlpers/api/PersistenceTransaction.java | 4 + .../impl/DefaultPersistenceManager.java | 26 +-- .../impl/DefaultPersistenceTransaction.java | 60 +++-- .../xmlpers/objref/LockableObject.java | 54 ++++- .../li/strolch/xmlpers/objref/ObjectRef.java | 8 +- .../xmlpers/test/AbstractPersistenceTest.java | 16 +- .../li/strolch/xmlpers/test/LockingTest.java | 24 +- 11 files changed, 306 insertions(+), 317 deletions(-) diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileDao.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileDao.java index 578903cf5..fcb310cd5 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileDao.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileDao.java @@ -115,54 +115,46 @@ public class FileDao { throw new IllegalArgumentException("IdRefs don't reference directories!"); //$NON-NLS-1$ } - objectRef.lock(); - - try { - - File directoryPath = objectRef.getPath(this.pathBuilder); - if (!directoryPath.getAbsolutePath().startsWith(this.pathBuilder.getRootPath().getAbsolutePath())) { - String msg = "The path for {0} is invalid as not child of {1}"; //$NON-NLS-1$ - msg = MessageFormat - .format(msg, directoryPath.getAbsolutePath(), this.pathBuilder.getRootPath().getAbsolutePath()); - throw new IllegalArgumentException(msg); - } - - if (!directoryPath.isDirectory()) { - String msg = "The path for {0} is not a directory: {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, objectRef.getName(), directoryPath.getAbsolutePath()); - throw new IllegalArgumentException(msg); - } - String[] list = directoryPath.list(); - if (list == null) { - String msg = "The path for {0} is not a directory: {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, objectRef.getName(), directoryPath.getAbsolutePath()); - throw new IllegalArgumentException(msg); - } - - // stop if empty - if (list.length != 0) - return; - - // delete - if (!directoryPath.delete()) { - String msg = "Deletion of empty directory for {0} at {1} failed! Check file permissions!"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, objectRef.getName(), directoryPath.getAbsolutePath()); - throw new XmlPersistenceException(msg); - } - - // log - if (this.verbose) { - String msg = "Deleted empty directory for {0} at {1}"; //$NON-NLS-1$ - logger.info(MessageFormat.format(msg, objectRef.getName(), directoryPath)); - } - - // recursively delete - ObjectRef parent = objectRef.getParent(this.tx); - deleteEmptyDirectories(parent); - - } finally { - objectRef.unlock(); + File directoryPath = objectRef.getPath(this.pathBuilder); + if (!directoryPath.getAbsolutePath().startsWith(this.pathBuilder.getRootPath().getAbsolutePath())) { + String msg = "The path for {0} is invalid as not child of {1}"; //$NON-NLS-1$ + msg = MessageFormat + .format(msg, directoryPath.getAbsolutePath(), this.pathBuilder.getRootPath().getAbsolutePath()); + throw new IllegalArgumentException(msg); } + + if (!directoryPath.isDirectory()) { + String msg = "The path for {0} is not a directory: {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, objectRef.getName(), directoryPath.getAbsolutePath()); + throw new IllegalArgumentException(msg); + } + String[] list = directoryPath.list(); + if (list == null) { + String msg = "The path for {0} is not a directory: {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, objectRef.getName(), directoryPath.getAbsolutePath()); + throw new IllegalArgumentException(msg); + } + + // stop if empty + if (list.length != 0) + return; + + // delete + if (!directoryPath.delete()) { + String msg = "Deletion of empty directory for {0} at {1} failed! Check file permissions!"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, objectRef.getName(), directoryPath.getAbsolutePath()); + throw new XmlPersistenceException(msg); + } + + // log + if (this.verbose) { + String msg = "Deleted empty directory for {0} at {1}"; //$NON-NLS-1$ + logger.info(MessageFormat.format(msg, objectRef.getName(), directoryPath)); + } + + // recursively delete + ObjectRef parent = objectRef.getParent(this.tx); + deleteEmptyDirectories(parent); } private void logPath(IoOperation operation, File path, ObjectRef objectRef) { @@ -175,16 +167,11 @@ public class FileDao { private void createMissingParents(File path, ObjectRef objectRef) { ObjectRef parentRef = objectRef.getParent(this.tx); - parentRef.lock(); - try { - File parentFile = parentRef.getPath(this.pathBuilder); - if (!parentFile.exists() && !parentFile.mkdirs()) { - String msg = "Could not create parent path for {0} at {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, objectRef.getName(), path.getAbsolutePath()); - throw new XmlPersistenceException(msg); - } - } finally { - parentRef.unlock(); + File parentFile = parentRef.getPath(this.pathBuilder); + if (!parentFile.exists() && !parentFile.mkdirs()) { + String msg = "Could not create parent path for {0} at {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, objectRef.getName(), path.getAbsolutePath()); + throw new XmlPersistenceException(msg); } } diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileIo.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileIo.java index 1378b3b14..27ccc6075 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileIo.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/FileIo.java @@ -26,9 +26,8 @@ import javax.xml.stream.XMLStreamWriter; import javax.xml.transform.*; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; -import java.io.File; -import java.io.FileWriter; -import java.io.IOException; +import java.io.*; +import java.nio.charset.Charset; import java.text.MessageFormat; import javanet.staxutils.IndentingXMLStreamWriter; @@ -59,27 +58,29 @@ public class FileIo { public void writeSax(PersistenceContext ctx) { - XMLStreamWriter writer; + XMLStreamWriter xmlWriter; try { - try (FileWriter fileWriter = new FileWriter(this.tmpPath)) { + try (Writer ioWriter = new OutputStreamWriter(new FileOutputStream(this.tmpPath), DEFAULT_ENCODING)) { XMLOutputFactory factory = XMLOutputFactory.newInstance(); - writer = factory.createXMLStreamWriter(fileWriter); - writer = new IndentingXMLStreamWriter(writer); + xmlWriter = factory.createXMLStreamWriter(ioWriter); + xmlWriter = new IndentingXMLStreamWriter(xmlWriter); // start document - writer.writeStartDocument(DEFAULT_ENCODING, DEFAULT_XML_VERSION); + xmlWriter.writeStartDocument(DEFAULT_ENCODING, DEFAULT_XML_VERSION); // then delegate object writing to caller SaxParser saxParser = ctx.getParserFactor().getSaxParser(); saxParser.setObject(ctx.getObject()); - saxParser.write(writer); + saxParser.write(xmlWriter); // and now end - writer.writeEndDocument(); - writer.flush(); + xmlWriter.writeEndDocument(); + xmlWriter.flush(); } + if (this.path.exists() && !this.path.delete()) + throw new IllegalStateException("Failed to delete existing file " + this.path.getAbsolutePath()); if (!this.tmpPath.renameTo(this.path)) { throw new IllegalStateException( "Failed to rename temp file " + this.tmpPath.getName() + " to " + this.path.getAbsolutePath()); @@ -158,21 +159,25 @@ public class FileIo { // transformer.setOutputProperty("{http://xml.apache.org/xalan}line-separator", "\t"); // Transform to file - StreamResult result = new StreamResult(this.tmpPath); - Source xmlSource = new DOMSource(document); - transformer.transform(xmlSource, result); + try (Writer ioWriter = new OutputStreamWriter(new FileOutputStream(this.tmpPath), encoding)) { + StreamResult result = new StreamResult(this.tmpPath); + Source xmlSource = new DOMSource(document); + transformer.transform(xmlSource, result); + } if (logger.isDebugEnabled()) { String msg = MessageFormat.format("Wrote DOM to {0}", this.tmpPath.getAbsolutePath()); //$NON-NLS-1$ logger.info(msg); } + if (this.path.exists() && !this.path.delete()) + throw new IllegalStateException("Failed to delete existing file " + this.path.getAbsolutePath()); if (!this.tmpPath.renameTo(this.path)) { throw new IllegalStateException( "Failed to rename temp file " + this.tmpPath.getName() + " to " + this.path.getAbsolutePath()); } - } catch (TransformerFactoryConfigurationError | TransformerException e) { + } catch (IOException | TransformerFactoryConfigurationError | TransformerException e) { if (this.tmpPath.exists()) { if (!this.tmpPath.delete()) logger.error("Failed to delete existing temp file " + this.tmpPath.getAbsolutePath()); diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/MetadataDao.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/MetadataDao.java index e59274e2d..a26364f3b 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/MetadataDao.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/MetadataDao.java @@ -49,21 +49,17 @@ public class MetadataDao { assertNotClosed(this.tx); assertNotIdRef(parentRef); - parentRef.lock(); - try { - File queryPath = parentRef.getPath(this.pathBuilder); - Set keySet = queryTypeSet(queryPath); + this.tx.lock(parentRef); + File queryPath = parentRef.getPath(this.pathBuilder); + Set keySet = queryTypeSet(queryPath); - if (this.verbose) { - String msg = "Found {0} types for {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, keySet.size(), parentRef.getName()); - logger.info(msg); - } - - return keySet; - } finally { - parentRef.unlock(); + if (this.verbose) { + String msg = "Found {0} types for {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, keySet.size(), parentRef.getName()); + logger.info(msg); } + + return keySet; } public Set queryKeySet(ObjectRef parentRef) { @@ -75,21 +71,17 @@ public class MetadataDao { assertNotRootRef(parentRef); assertNotIdRef(parentRef); - parentRef.lock(); - try { - File queryPath = parentRef.getPath(this.pathBuilder); - Set keySet = queryKeySet(queryPath, reverse); + this.tx.lock(parentRef); + File queryPath = parentRef.getPath(this.pathBuilder); + Set keySet = queryKeySet(queryPath, reverse); - if (this.verbose) { - String msg = "Found {0} objects for {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, keySet.size(), parentRef.getName()); - logger.info(msg); - } - - return keySet; - } finally { - parentRef.unlock(); + if (this.verbose) { + String msg = "Found {0} objects for {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, keySet.size(), parentRef.getName()); + logger.info(msg); } + + return keySet; } public long queryTypeSize(ObjectRef parentRef) { @@ -97,41 +89,33 @@ public class MetadataDao { assertNotRootRef(parentRef); assertNotIdRef(parentRef); - parentRef.lock(); - try { - File queryPath = parentRef.getPath(this.pathBuilder); - long numberOfFiles = queryTypeSize(queryPath); + this.tx.lock(parentRef); + File queryPath = parentRef.getPath(this.pathBuilder); + long numberOfFiles = queryTypeSize(queryPath); - if (this.verbose) { - String msg = "Found {0} types for {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, numberOfFiles, parentRef.getName()); - logger.info(msg); - } - - return numberOfFiles; - } finally { - parentRef.unlock(); + if (this.verbose) { + String msg = "Found {0} types for {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, numberOfFiles, parentRef.getName()); + logger.info(msg); } + + return numberOfFiles; } public long querySize(ObjectRef parentRef) { assertNotClosed(this.tx); - parentRef.lock(); - try { - File queryPath = parentRef.getPath(this.pathBuilder); - long numberOfFiles = querySize(queryPath); + this.tx.lock(parentRef); + File queryPath = parentRef.getPath(this.pathBuilder); + long numberOfFiles = querySize(queryPath); - if (this.verbose) { - String msg = "Found {0} objects for {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, numberOfFiles, parentRef.getName()); - logger.info(msg); - } - - return numberOfFiles; - } finally { - parentRef.unlock(); + if (this.verbose) { + String msg = "Found {0} objects for {1}"; //$NON-NLS-1$ + msg = MessageFormat.format(msg, numberOfFiles, parentRef.getName()); + logger.info(msg); } + + return numberOfFiles; } /** diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/ObjectDao.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/ObjectDao.java index 0ed2bc3bc..63d14bc19 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/ObjectDao.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/ObjectDao.java @@ -45,19 +45,22 @@ public class ObjectDao { assertNotClosed(); assertNotNull(object); PersistenceContext ctx = createCtx(object); - ctx.getObjectRef().lock(); + this.tx.lock(ctx.getObjectRef().getParent(this.tx)); + this.tx.lock(ctx.getObjectRef()); this.objectFilter.add(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); } public void addAll(List objects) { assertNotClosed(); assertNotNull(objects); - if (!objects.isEmpty()) { - for (T object : objects) { - PersistenceContext ctx = createCtx(object); - ctx.getObjectRef().lock(); - this.objectFilter.add(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); - } + if (objects.isEmpty()) + return; + + for (T object : objects) { + PersistenceContext ctx = createCtx(object); + this.tx.lock(ctx.getObjectRef().getParent(this.tx)); + this.tx.lock(ctx.getObjectRef()); + this.objectFilter.add(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); } } @@ -65,19 +68,20 @@ public class ObjectDao { assertNotClosed(); assertNotNull(object); PersistenceContext ctx = createCtx(object); - ctx.getObjectRef().lock(); + this.tx.lock(ctx.getObjectRef()); this.objectFilter.update(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); } public void updateAll(List objects) { assertNotClosed(); assertNotNull(objects); - if (!objects.isEmpty()) { - for (T object : objects) { - PersistenceContext ctx = createCtx(object); - ctx.getObjectRef().lock(); - this.objectFilter.update(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); - } + if (objects.isEmpty()) + return; + + for (T object : objects) { + PersistenceContext ctx = createCtx(object); + this.tx.lock(ctx.getObjectRef()); + this.objectFilter.update(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); } } @@ -85,19 +89,22 @@ public class ObjectDao { assertNotClosed(); assertNotNull(object); PersistenceContext ctx = createCtx(object); - ctx.getObjectRef().lock(); + this.tx.lock(ctx.getObjectRef().getParent(this.tx)); + this.tx.lock(ctx.getObjectRef()); this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); } public void removeAll(List objects) { assertNotClosed(); assertNotNull(objects); - if (!objects.isEmpty()) { - for (T object : objects) { - PersistenceContext ctx = createCtx(object); - ctx.getObjectRef().lock(); - this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); - } + if (objects.isEmpty()) + return; + + for (T object : objects) { + PersistenceContext ctx = createCtx(object); + this.tx.lock(ctx.getObjectRef().getParent(this.tx)); + this.tx.lock(ctx.getObjectRef()); + this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); } } @@ -106,31 +113,22 @@ public class ObjectDao { long removed = 0; - Set refs = new TreeSet<>(); - typeRef.lock(); - refs.add(typeRef); - try { + this.tx.lock(typeRef); - Set types = this.tx.getMetadataDao().queryTypeSet(typeRef); - for (String type : types) { - ObjectRef childTypeRef = typeRef.getChildTypeRef(this.tx, type); - childTypeRef.lock(); - refs.add(childTypeRef); + Set types = this.tx.getMetadataDao().queryTypeSet(typeRef); + for (String type : types) { + ObjectRef childTypeRef = typeRef.getChildTypeRef(this.tx, type); + this.tx.lock(childTypeRef); - Set ids = queryKeySet(childTypeRef, false); - for (String id : ids) { + Set ids = queryKeySet(childTypeRef, false); + for (String id : ids) { - ObjectRef idRef = childTypeRef.getChildIdRef(this.tx, id); + ObjectRef idRef = childTypeRef.getChildIdRef(this.tx, id); - PersistenceContext ctx = createCtx(idRef); - ctx.getObjectRef().lock(); - this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); - removed++; - } - } - } finally { - for (ObjectRef ref : refs) { - ref.unlock(); + PersistenceContext ctx = createCtx(idRef); + this.tx.lock(ctx.getObjectRef()); + this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); + removed++; } } @@ -144,20 +142,17 @@ public class ObjectDao { long removed = 0; - subTypeRef.lock(); - try { - Set ids = queryKeySet(subTypeRef, false); - for (String id : ids) { + this.tx.lock(subTypeRef); - ObjectRef idRef = subTypeRef.getChildIdRef(this.tx, id); + Set ids = queryKeySet(subTypeRef, false); + for (String id : ids) { - PersistenceContext ctx = createCtx(idRef); - ctx.getObjectRef().lock(); - this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); - removed++; - } - } finally { - subTypeRef.unlock(); + ObjectRef idRef = subTypeRef.getChildIdRef(this.tx, id); + + PersistenceContext ctx = createCtx(idRef); + this.tx.lock(ctx.getObjectRef()); + this.objectFilter.remove(ctx.getObjectRef().getType(), ctx.getObjectRef(), ctx); + removed++; } return removed; @@ -167,7 +162,8 @@ public class ObjectDao { assertNotClosed(); assertIsIdRef(objectRef); PersistenceContext ctx = createCtx(objectRef); - ctx.getObjectRef().lock(); + this.tx.lock(ctx.getObjectRef().getParent(this.tx)); + this.tx.lock(ctx.getObjectRef()); this.objectFilter.remove(objectRef.getType(), ctx.getObjectRef(), ctx); } @@ -176,19 +172,15 @@ public class ObjectDao { assertIsNotIdRef(parentRef); assertIsNotRootRef(parentRef); - parentRef.lock(); - try { + this.tx.lock(parentRef); - Set keySet = queryKeySet(parentRef, false); - for (String id : keySet) { + Set keySet = queryKeySet(parentRef, false); + for (String id : keySet) { - ObjectRef childRef = parentRef.getChildIdRef(this.tx, id); - PersistenceContext ctx = createCtx(childRef); - ctx.getObjectRef().lock(); - this.objectFilter.remove(childRef.getType(), ctx.getObjectRef(), ctx); - } - } finally { - parentRef.unlock(); + ObjectRef childRef = parentRef.getChildIdRef(this.tx, id); + PersistenceContext ctx = createCtx(childRef); + this.tx.lock(ctx.getObjectRef()); + this.objectFilter.remove(childRef.getType(), ctx.getObjectRef(), ctx); } } @@ -196,27 +188,19 @@ public class ObjectDao { assertNotClosed(); assertIsIdRef(objectRef); - objectRef.lock(); - try { - PersistenceContext ctx = objectRef.createPersistenceContext(this.tx); - return this.fileDao.exists(ctx); - } finally { - objectRef.unlock(); - } + this.tx.lock(objectRef); + PersistenceContext ctx = objectRef.createPersistenceContext(this.tx); + return this.fileDao.exists(ctx); } public T queryById(ObjectRef objectRef) { assertNotClosed(); assertIsIdRef(objectRef); - objectRef.lock(); - try { - PersistenceContext ctx = objectRef.createPersistenceContext(this.tx); - this.fileDao.performRead(ctx); - return ctx.getObject(); - } finally { - objectRef.unlock(); - } + this.tx.lock(objectRef); + PersistenceContext ctx = objectRef.createPersistenceContext(this.tx); + this.fileDao.performRead(ctx); + return ctx.getObject(); } public List queryAll(ObjectRef parentRef) { @@ -227,62 +211,45 @@ public class ObjectDao { assertNotClosed(); assertIsNotIdRef(parentRef); - parentRef.lock(); - try { + this.tx.lock(parentRef); - MetadataDao metadataDao = this.tx.getMetadataDao(); - Set keySet = metadataDao.queryKeySet(parentRef, reverse); + MetadataDao metadataDao = this.tx.getMetadataDao(); + Set keySet = metadataDao.queryKeySet(parentRef, reverse); - int i = 0; - List result = new ArrayList<>(); - for (String id : keySet) { + int i = 0; + List result = new ArrayList<>(); + for (String id : keySet) { - ObjectRef childRef = parentRef.getChildIdRef(this.tx, id); - PersistenceContext childCtx = childRef.createPersistenceContext(this.tx); - childCtx.getObjectRef().lock(); - try { - this.fileDao.performRead(childCtx); - assertObjectRead(childCtx); - result.add(childCtx.getObject()); - } finally { - childCtx.getObjectRef().unlock(); - } + ObjectRef childRef = parentRef.getChildIdRef(this.tx, id); + PersistenceContext childCtx = childRef.createPersistenceContext(this.tx); + this.tx.lock(childCtx.getObjectRef()); + this.fileDao.performRead(childCtx); + assertObjectRead(childCtx); + result.add(childCtx.getObject()); - if (maxSize != Integer.MAX_VALUE && i >= maxSize) - break; - } - - return result; - - } finally { - parentRef.unlock(); + if (maxSize != Integer.MAX_VALUE && i >= maxSize) + break; } + + return result; } private Set queryKeySet(ObjectRef parentRef, boolean reverse) { assertNotClosed(); assertIsNotIdRef(parentRef); - parentRef.lock(); - try { - MetadataDao metadataDao = this.tx.getMetadataDao(); - return metadataDao.queryKeySet(parentRef, reverse); - } finally { - parentRef.unlock(); - } + this.tx.lock(parentRef); + MetadataDao metadataDao = this.tx.getMetadataDao(); + return metadataDao.queryKeySet(parentRef, reverse); } public long querySize(ObjectRef parentRef) { assertNotClosed(); assertIsNotIdRef(parentRef); - parentRef.lock(); - try { - MetadataDao metadataDao = this.tx.getMetadataDao(); - return metadataDao.querySize(parentRef); - } finally { - parentRef.unlock(); - } + this.tx.lock(parentRef); + MetadataDao metadataDao = this.tx.getMetadataDao(); + return metadataDao.querySize(parentRef); } public PersistenceContext createCtx(T object) { diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/PersistenceTransaction.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/PersistenceTransaction.java index 9051da101..f4a03af47 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/PersistenceTransaction.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/api/PersistenceTransaction.java @@ -15,6 +15,8 @@ */ package li.strolch.xmlpers.api; +import li.strolch.xmlpers.objref.LockableObject; + /** * @author Robert von Burg */ @@ -61,4 +63,6 @@ public interface PersistenceTransaction extends AutoCloseable { public FileDao getFileDao(); public PersistenceManager getManager(); + + void lock(LockableObject lockableObject); } \ No newline at end of file diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceManager.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceManager.java index 1dc1b108d..ac5bbd4d3 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceManager.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceManager.java @@ -15,6 +15,9 @@ */ package li.strolch.xmlpers.impl; +import static li.strolch.utils.helper.PropertiesHelper.*; +import static li.strolch.xmlpers.api.PersistenceConstants.*; + import java.io.File; import java.lang.reflect.Field; import java.text.MessageFormat; @@ -49,25 +52,14 @@ public class DefaultPersistenceManager implements PersistenceManager { String context = DefaultPersistenceManager.class.getSimpleName(); // get properties - boolean verbose = PropertiesHelper - .getPropertyBool(properties, context, PersistenceConstants.PROP_VERBOSE, Boolean.FALSE); - String ioModeS = PropertiesHelper - .getProperty(properties, context, PersistenceConstants.PROP_XML_IO_MOD, IoMode.DOM.name()); - IoMode ioMode = IoMode.valueOf(ioModeS); - long lockTime = PropertiesHelper - .getPropertyLong(properties, context, PersistenceConstants.PROP_LOCK_TIME_MILLIS, 10000L); - String basePath = PropertiesHelper.getProperty(properties, context, PersistenceConstants.PROP_BASEPATH, null); + boolean verbose = getPropertyBool(properties, context, PROP_VERBOSE, Boolean.FALSE); + IoMode ioMode = IoMode.valueOf(getProperty(properties, context, PROP_XML_IO_MOD, IoMode.DOM.name())); + long lockTime = getPropertyLong(properties, context, PROP_LOCK_TIME_MILLIS, LockableObject.getLockTime()); + String basePath = getProperty(properties, context, PROP_BASEPATH, null); // set lock time on LockableObject - try { - Field lockTimeField = LockableObject.class.getDeclaredField("tryLockTime");//$NON-NLS-1$ - lockTimeField.setAccessible(true); - lockTimeField.setLong(null, lockTime); - logger.info("Using a max lock acquire time of " + StringHelper - .formatMillisecondsDuration(lockTime)); //$NON-NLS-1$ - } catch (SecurityException | NoSuchFieldException | IllegalArgumentException | IllegalAccessException e) { - throw new RuntimeException("Failed to configure tryLockTime on LockableObject!", e); //$NON-NLS-1$ - } + if (lockTime != LockableObject.getLockTime()) + LockableObject.setTryLockTime(lockTime); // validate base path exists and is writable File basePathF = new File(basePath).getAbsoluteFile(); diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceTransaction.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceTransaction.java index ba82e2eb2..b20977c2b 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceTransaction.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/impl/DefaultPersistenceTransaction.java @@ -22,6 +22,7 @@ import java.util.*; import li.strolch.utils.objectfilter.ObjectFilter; import li.strolch.xmlpers.api.*; +import li.strolch.xmlpers.objref.LockableObject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,6 +49,8 @@ public class DefaultPersistenceTransaction implements PersistenceTransaction { private Date startTimeDate; private TransactionResult txResult; + private Set lockedObjects; + public DefaultPersistenceTransaction(PersistenceManager manager, IoMode ioMode, boolean verbose) { this.startTime = System.nanoTime(); this.startTimeDate = new Date(); @@ -60,6 +63,7 @@ public class DefaultPersistenceTransaction implements PersistenceTransaction { this.closeStrategy = TransactionCloseStrategy.COMMIT; this.state = TransactionState.OPEN; + this.lockedObjects = new HashSet<>(); } @Override @@ -113,27 +117,31 @@ public class DefaultPersistenceTransaction implements PersistenceTransaction { @Override public void autoCloseableRollback() { - long start = System.nanoTime(); - if (this.state == TransactionState.COMMITTED) - throw new IllegalStateException("Transaction has already been committed!"); //$NON-NLS-1$ + try { + long start = System.nanoTime(); + if (this.state == TransactionState.COMMITTED) + throw new IllegalStateException("Transaction has already been committed!"); //$NON-NLS-1$ - if (this.state != TransactionState.ROLLED_BACK) { - unlockObjectRefs(); - this.state = TransactionState.ROLLED_BACK; - this.objectFilter.clearCache(); + if (this.state != TransactionState.ROLLED_BACK) { + this.state = TransactionState.ROLLED_BACK; - long end = System.nanoTime(); - long txDuration = end - this.startTime; - long closeDuration = end - start; + long end = System.nanoTime(); + long txDuration = end - this.startTime; + long closeDuration = end - start; - if (this.txResult != null) { - this.txResult.clear(); - this.txResult.setState(this.state); - this.txResult.setStartTime(this.startTimeDate); - this.txResult.setTxDuration(txDuration); - this.txResult.setCloseDuration(closeDuration); - this.txResult.setModificationByKey(Collections.emptyMap()); + if (this.txResult != null) { + this.txResult.clear(); + this.txResult.setState(this.state); + this.txResult.setStartTime(this.startTimeDate); + this.txResult.setTxDuration(txDuration); + this.txResult.setCloseDuration(closeDuration); + this.txResult.setModificationByKey(Collections.emptyMap()); + } } + } finally { + // clean up + this.objectFilter.clearCache(); + releaseAllLocks(); } } @@ -242,10 +250,9 @@ public class DefaultPersistenceTransaction implements PersistenceTransaction { this.txResult.setFailCause(e); } finally { - // clean up - unlockObjectRefs(); this.objectFilter.clearCache(); + releaseAllLocks(); } long end = System.nanoTime(); @@ -274,16 +281,21 @@ public class DefaultPersistenceTransaction implements PersistenceTransaction { } } - @SuppressWarnings("rawtypes") - private void unlockObjectRefs() { - List lockedObjects = this.objectFilter.getAll(PersistenceContext.class); - for (PersistenceContext lockedObject : lockedObjects) { - lockedObject.getObjectRef().unlock(); + private void releaseAllLocks() { + for (LockableObject lockedObject : this.lockedObjects) { + lockedObject.releaseLock(); } + this.lockedObjects.clear(); } @Override public boolean isOpen() { return this.state == TransactionState.OPEN; } + + @Override + public void lock(LockableObject lockableObject) { + lockableObject.lock(); + this.lockedObjects.add(lockableObject); + } } diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/LockableObject.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/LockableObject.java index 2ea584b65..0154a3daa 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/LockableObject.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/LockableObject.java @@ -15,7 +15,12 @@ */ package li.strolch.xmlpers.objref; +import static java.lang.Thread.currentThread; +import static java.text.MessageFormat.format; +import static li.strolch.utils.helper.StringHelper.formatMillisecondsDuration; + import java.text.MessageFormat; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReentrantLock; @@ -29,12 +34,22 @@ public class LockableObject { private static final Logger logger = LoggerFactory.getLogger(LockableObject.class); private static long tryLockTime = 10000L; - private final ReentrantLock lock; + public static void setTryLockTime(long tryLockTime) { + LockableObject.tryLockTime = tryLockTime; + } - public LockableObject() { + private final ReentrantLock lock; + protected final String name; + + public LockableObject(String name) { + this.name = name; this.lock = new ReentrantLock(true); } + public String getName() { + return this.name; + } + public static long getLockTime() { return tryLockTime; } @@ -43,11 +58,32 @@ public class LockableObject { * @see java.util.concurrent.locks.ReentrantLock#tryLock(long, TimeUnit) */ public void lock() { + + // don't lock multiple times + if (this.lock.isHeldByCurrentThread() && this.lock.isLocked()) + return; + try { if (!this.lock.tryLock(tryLockTime, TimeUnit.MILLISECONDS)) { - String msg = "Failed to acquire lock after {0} for {1}"; //$NON-NLS-1$ - msg = MessageFormat.format(msg, StringHelper.formatMillisecondsDuration(tryLockTime), toString()); + String msg = "Thread {0} failed to acquire lock after {1} for {2}"; //$NON-NLS-1$ + msg = format(msg, currentThread().getName(), formatMillisecondsDuration(tryLockTime), this); + + try { + logger.error(msg); + logger.error("Listing all active threads: "); + Map allStackTraces = Thread.getAllStackTraces(); + for (Thread thread : allStackTraces.keySet()) { + StackTraceElement[] trace = allStackTraces.get(thread); + StringBuilder sb = new StringBuilder(); + for (StackTraceElement traceElement : trace) + sb.append("\n\tat ").append(traceElement); + logger.error("\nThread " + thread.getName() + ":\n" + sb.toString() + "\n"); + } + } catch (Exception e) { + logger.error("Failed to log active threads: " + e.getMessage(), e); + } + throw new XmlPersistenceException(msg); } if (logger.isDebugEnabled()) @@ -60,9 +96,11 @@ public class LockableObject { /** * @see java.util.concurrent.locks.ReentrantLock#unlock() */ - public void unlock() { - this.lock.unlock(); - if (logger.isDebugEnabled()) - logger.debug("unlocking " + toString()); //$NON-NLS-1$ + public void releaseLock() { + while (this.lock.isHeldByCurrentThread() && this.lock.isLocked()) { + if (logger.isDebugEnabled()) + logger.debug("unlocking " + toString()); //$NON-NLS-1$ + this.lock.unlock(); + } } } diff --git a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/ObjectRef.java b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/ObjectRef.java index b558d7c1a..aa534a6d8 100644 --- a/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/ObjectRef.java +++ b/li.strolch.xmlpers/src/main/java/li/strolch/xmlpers/objref/ObjectRef.java @@ -24,14 +24,8 @@ import li.strolch.xmlpers.impl.PathBuilder; public abstract class ObjectRef extends LockableObject implements Comparable { - protected final String name; - protected ObjectRef(String name) { - this.name = name; - } - - public String getName() { - return this.name; + super(name); } public File getPath(PathBuilder pathBuilder) { diff --git a/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/AbstractPersistenceTest.java b/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/AbstractPersistenceTest.java index 96beda5c9..9e048eba9 100644 --- a/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/AbstractPersistenceTest.java +++ b/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/AbstractPersistenceTest.java @@ -15,10 +15,14 @@ */ package li.strolch.xmlpers.test; +import static li.strolch.utils.helper.SystemHelper.isLinux; +import static li.strolch.utils.helper.SystemHelper.isWindows; + import java.io.File; import java.util.Properties; import li.strolch.utils.helper.FileHelper; +import li.strolch.utils.helper.SystemHelper; import li.strolch.xmlpers.api.IoMode; import li.strolch.xmlpers.api.PersistenceConstants; import li.strolch.xmlpers.api.PersistenceManager; @@ -42,8 +46,14 @@ public abstract class AbstractPersistenceTest { File file = new File(path).getAbsoluteFile(); File parent = file.getParentFile(); - if (!parent.getAbsolutePath().endsWith("/target/db")) - throw new RuntimeException("Bad parent! Must be /target/db/: " + parent); + if (isWindows()) { + if (!parent.getAbsolutePath().endsWith("\\target\\db")) + throw new RuntimeException("Bad parent! Must be \\target\\db: " + parent); + } else { + if (!parent.getAbsolutePath().endsWith("/target/db")) + throw new RuntimeException("Bad parent! Must be /target/db: " + parent); + } + if (!parent.exists() && !parent.mkdirs()) throw new RuntimeException("Failed to create path " + parent); @@ -51,7 +61,7 @@ public abstract class AbstractPersistenceTest { if (!FileHelper.deleteFiles(file.listFiles(), true)) throw new RuntimeException("Could not clean up path " + file.getAbsolutePath()); - if (!file.exists() && !file.mkdir()) + if (!file.exists() && !file.mkdirs()) throw new RuntimeException("Failed to create path " + file); File domFile = new File(file, IoMode.DOM.name()); diff --git a/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/LockingTest.java b/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/LockingTest.java index a81581926..3a4ba8a99 100644 --- a/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/LockingTest.java +++ b/li.strolch.xmlpers/src/test/java/li/strolch/xmlpers/test/LockingTest.java @@ -57,7 +57,7 @@ public class LockingTest extends AbstractPersistenceTest { properties.setProperty(PersistenceConstants.PROP_LOCK_TIME_MILLIS, Long.toString(500L)); setup(properties); - this.waitForWorkersTime = LockableObject.getLockTime() + getWaitForWorkersTime() + 300L; + this.waitForWorkersTime = LockableObject.getLockTime() + 300L; } @Test @@ -112,15 +112,18 @@ public class LockingTest extends AbstractPersistenceTest { logger.info("Setup thread " + worker.getName()); //$NON-NLS-1$ } - // create resource which is to be updated + int nrOfSuccess; try (PersistenceTransaction tx = this.persistenceManager.openTx()) { + + // create resource which is to be updated MyModel resource = createResource(resourceId); tx.getObjectDao().add(resource); + + // and before closing the TX, run the workers, which should fail as we are still holding the locks + nrOfSuccess = runWorkers(workers); } - int nrOfSuccess = runWorkers(workers); - - assertEquals("Only one thread should be able to perform the TX!", 1, nrOfSuccess); //$NON-NLS-1$ + assertEquals("Only one thread should be able to perform the TX!", 0, nrOfSuccess); //$NON-NLS-1$ } private int runWorkers(List workers) throws InterruptedException { @@ -128,7 +131,7 @@ public class LockingTest extends AbstractPersistenceTest { setRun(true); for (AbstractWorker worker : workers) { - worker.join(getWaitForWorkersTime() + 2000L); + worker.join(getWaitForWorkersTime() + 5000L); } int nrOfSuccess = 0; @@ -177,16 +180,9 @@ public class LockingTest extends AbstractPersistenceTest { logger.info("Starting work..."); //$NON-NLS-1$ try (PersistenceTransaction tx = LockingTest.this.persistenceManager.openTx()) { doWork(tx); - - try { - Thread.sleep(getWaitForWorkersTime()); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - - this.success = true; } + this.success = true; logger.info("Work completed."); //$NON-NLS-1$ }