[Exist-commits] eXist-1.0/src/org/exist/collections Collection.java,1.43,1.44 CollectionCache.java,1.6,1.7

[Wolfgang M. M. <wol...@us...>]

Update of /cvsroot/exist/eXist-1.0/src/org/exist/collections
In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv8244/src/org/exist/collections

Modified Files:
	Collection.java CollectionCache.java 
Log Message:
Revised collection locking to fix various concurrency errors. It is now the responsibility of the 
caller to lock/unlock a collection. A collection can be retrieved and locked via the new 
openCollection() method provided by DBBroker. After reading/modifying the collection, it should be 
unlocked by calling Collection.release.

The local XML:DB implementation and the XMLRPC interface have been changed to reflect the new
collection locking rules.

Index: CollectionCache.java
===================================================================
RCS file: /cvsroot/exist/eXist-1.0/src/org/exist/collections/CollectionCache.java,v
retrieving revision 1.6
retrieving revision 1.7
diff -C2 -d -r1.6 -r1.7
*** CollectionCache.java	25 May 2004 12:52:01 -0000	1.6
--- CollectionCache.java	3 Aug 2004 15:25:58 -0000	1.7
***************
*** 3,6 ****
--- 3,7 ----
  import org.exist.storage.cache.Cacheable;
  import org.exist.storage.cache.LRDCache;
+ import org.exist.util.Lock;
  import org.exist.util.hashtable.Object2LongHashMap;
  
***************
*** 21,25 ****
  		names = new Object2LongHashMap(blockBuffers);
  	}
! 
  	public void add(Collection collection) {
  		add(collection, 1);
--- 22,26 ----
  		names = new Object2LongHashMap(blockBuffers);
  	}
! 	
  	public void add(Collection collection) {
  		add(collection, 1);
***************
*** 42,50 ****
  	}
  
  	protected Cacheable removeOne(Cacheable item) {
! 		Cacheable old = super.removeOne(item);
! 		if(old != null) {
! 			names.remove(((Collection)old).getName());
  		}
  		return old;
  	}
--- 43,86 ----
  	}
  
+ 	/**
+ 	 * Overwritten to lock collections before they are removed.
+ 	 */
  	protected Cacheable removeOne(Cacheable item) {
! 		Collection old;
! 		Lock lock;
! 		double rd = 0, minRd = -1;
! 		int bucket = -1;
! 		boolean unloadable;
! 		for (int i = 0; i < items.length; i++) {
! 			old = (Collection)items[i];
! 			if (old == null) {
! 				bucket = i;
! 				break;
! 			} else {
! 				lock = old.getLock(); 
! 					// calculate the reference density
! 					rd =
! 						old.getReferenceCount()
! 							/ (double)(totalReferences - old.getTimestamp());
! 					// attempt to acquire a read lock on the collection.
! 					// the collection is not considered for removal if the lock 
! 					// cannot be acquired immediately.
! 					if(lock.attempt(Lock.READ_LOCK)) {
! 						if ((minRd < 0 || rd < minRd) && old.allowUnload()) {
! 							minRd = rd;
! 							bucket = i;
! 						}
! 						lock.release();
! 					}
! 			}
  		}
+ 		old = (Collection)items[bucket];
+ 		if (old != null) {
+ 			map.remove(old.getKey());
+ 			names.remove(old.getName());
+ 			old.sync();
+ 		}
+ 		items[bucket] = item;
+ 		map.put(item.getKey(), item);
  		return old;
  	}

Index: Collection.java
===================================================================
RCS file: /cvsroot/exist/eXist-1.0/src/org/exist/collections/Collection.java,v
retrieving revision 1.43
retrieving revision 1.44
diff -C2 -d -r1.43 -r1.44
*** Collection.java	28 Jul 2004 19:40:42 -0000	1.43
--- Collection.java	3 Aug 2004 15:25:58 -0000	1.44
***************
*** 77,83 ****
  
  /**
!  * This class represents a collection in the database.
   * 
!  * A collection maintains a list of sub-collections and documents.
   *  
   * @author wolf
--- 77,86 ----
  
  /**
!  * This class represents a collection in the database. A collection maintains a list of 
!  * sub-collections and documents, and provides the methods to store/remove resources.
   * 
!  * Collections are shared between {@link org.exist.storage.DBBroker} instances. The caller
!  * is responsible to lock/unlock the collection. Call {@link DBBroker#openCollection(String, int)}  
!  * to get a collection with a read or write lock and {@link #release()} to release the lock.
   *  
   * @author wolf
***************
*** 149,152 ****
--- 152,159 ----
  	}
  	
+ 	public Lock getLock() {
+ 		return lock;
+ 	}
+ 	
  	/**
  	 *  Add a new sub-collection to the collection.
***************
*** 161,164 ****
--- 168,184 ----
  	}
  
+ 	public boolean hasChildCollection(String name) {
+ 		return subcollections.contains(name);
+ 	}
+ 	
+ 	/**
+ 	 * Closes the collection, i.e. releases the lock held by 
+ 	 * the current thread. This is a shortcut for getLock().release().
+ 	 */
+ 	public void release() {
+ //		LOG.debug("releasing lock on " + name);
+ 		lock.release();
+ 	}
+ 	
  	/**
  	 * Update the specified child-collection.
***************
*** 272,278 ****
  			boolean recursive, boolean checkPermissions) {
  		if (permissions.validate(broker.getUser(), Permission.READ)) {
! 			getDocuments(broker, docs, checkPermissions);
! 			if (recursive)
! 				allDocs(broker, docs, checkPermissions);
  		}
  		return docs;
--- 292,301 ----
  			boolean recursive, boolean checkPermissions) {
  		if (permissions.validate(broker.getUser(), Permission.READ)) {
! 			CollectionCache cache = broker.getCollectionsCache();
! 			synchronized (cache) {
! 				getDocuments(broker, docs, checkPermissions);
! 				if (recursive)
! 					allDocs(broker, docs, checkPermissions);
! 			}
  		}
  		return docs;
***************
*** 290,294 ****
  				if(child == null) {
  					LOG.warn("child collection " + childName + " not found. Skipping ...");
! 				// we always check if we have permissions to read the child collection
  				} else if (child.permissions.validate(broker.getUser(), Permission.READ)) {
  					child.getDocuments(broker, docs, checkPermissions);
--- 313,317 ----
  				if(child == null) {
  					LOG.warn("child collection " + childName + " not found. Skipping ...");
! 					// we always check if we have permissions to read the child collection
  				} else if (child.permissions.validate(broker.getUser(), Permission.READ)) {
  					child.getDocuments(broker, docs, checkPermissions);
***************
*** 446,449 ****
--- 469,498 ----
  
  	/**
+ 	 * Retrieve a child resource after putting a read lock on it. With this method,
+ 	 * access to the received document object is safe. 
+ 	 * 
+ 	 * @param broker
+ 	 * @param name
+ 	 * @return
+ 	 * @throws LockException
+ 	 */
+ 	public DocumentImpl getDocumentWithLock(DBBroker broker, String name, int lockMode) 
+ 	throws LockException {
+ 	    try {
+ 	        lock.acquire(Lock.READ_LOCK);
+ 	        if(reloadRequired) {
+ 			    broker.reloadCollection(this);
+ 			    reloadRequired = false;
+ 			}
+ 	        DocumentImpl doc = (DocumentImpl) documents.get(name);
+ 	        Lock updateLock = doc.getUpdateLock();
+ 	        updateLock.acquire(lockMode);
+ 	        return doc;
+         } finally {
+ 	        lock.release();
+ 	    }
+ 	}
+ 	
+ 	/**
  	 * Release any locks held on the document.
  	 * 
***************
*** 497,506 ****
  	 *is the root collection.
  	 */
! 	public Collection getParent(DBBroker broker) {
  		if (name.equals("/db"))
  			return null;
  		String parent = (name.lastIndexOf("/") < 1 ? "/db" : name.substring(0,
  				name.lastIndexOf("/")));
! 		return broker.getCollection(parent);
  	}
  
--- 546,555 ----
  	 *is the root collection.
  	 */
! 	public String getParentPath() {
  		if (name.equals("/db"))
  			return null;
  		String parent = (name.lastIndexOf("/") < 1 ? "/db" : name.substring(0,
  				name.lastIndexOf("/")));
! 		return parent;
  	}
  
***************
*** 663,667 ****
  						doc);
  			}
! 			broker.removeDocument(getName() + '/' + docname);
  			documents.remove(docname);
  			broker.saveCollection(this);
--- 712,716 ----
  						doc);
  			}
! 			broker.removeDocument(doc);
  			documents.remove(docname);
  			broker.saveCollection(this);
***************
*** 715,718 ****
--- 764,893 ----
  	}
  
+ 	public Indexer validate(DBBroker broker, String name, InputSource source) 
+ 	throws EXistException, PermissionDeniedException, TriggerException,
+ 	SAXException, LockException {
+ 		if (broker.isReadOnly())
+ 			throw new PermissionDeniedException("Database is read-only");
+ 		try {
+ 			lock.acquire(Lock.WRITE_LOCK);
+ 			XMLReader currentReader = getReader(broker);
+ 			DocumentImpl oldDoc = (DocumentImpl)documents.get(name);
+ 			DocumentImpl document = new DocumentImpl(broker, name, this);
+ 			// first pass: parse the document to determine tree structure
+ 			return determineTreeStructure(broker, name, document, oldDoc, currentReader, source);
+ 		} finally {
+ 			lock.release();
+ 		}
+ 	}
+ 	
+ 	public void store(DBBroker broker, Indexer indexer, InputSource source, boolean privileged)
+ 	throws EXistException, PermissionDeniedException, TriggerException,
+ 	SAXException, LockException {
+ 		// reset the input source
+ 		try {
+ 			final InputStream is = source.getByteStream();
+ 			if (is != null)
+ 				is.reset();
+ 			else {
+ 				final Reader cs = source.getCharacterStream();
+ 				if (cs != null)
+ 					cs.reset();
+ 			}
+ 		} catch (IOException e) {
+ 			LOG.debug("could not reset input source", e);
+ 		}
+ 		// second pass: store the document
+ 		DocumentImpl document = indexer.getDocument();
+ 		LOG.debug("storing document " + document.getDocId() + "; " + document.getFileName() + " ...");
+ 		try {
+ 			try {
+ 				indexer.getReader().parse(source);
+ 			} catch (IOException e) {
+ 				throw new EXistException(e);
+ 			}
+ 			if(!hasDocument(document.getFileName())) {
+ 				addDocument(broker, document);
+ 				broker.addDocument(this, document);
+ 			} else {
+ 				broker.updateDocument(document);
+ 			}
+ 			broker.closeDocument();
+ 			broker.flush();
+ 			broker.checkTree(document);
+ 			LOG.debug("document stored.");
+ 			// if we are running in privileged mode (e.g. backup/restore)
+ 			// notify the SecurityManager about changes
+ 			if (getName().equals(SecurityManager.SYSTEM) && document.getFileName().equals(SecurityManager.ACL_FILE)
+ 			        && privileged == false) {
+ 			    // inform the security manager that system data has changed
+ 			    LOG.debug("users.xml changed");
+ 			    broker.getBrokerPool().reloadSecurityManager(broker);
+ 			}
+ 		} finally {
+ 			document.getUpdateLock().release(Lock.WRITE_LOCK);
+ 		}
+ 		broker.deleteObservers();
+ 		return;
+ 	}
+ 	
+ 	public Indexer validate(DBBroker broker, String name, String data)
+ 	throws EXistException, PermissionDeniedException, TriggerException,
+ 	SAXException, LockException {
+ 		if (broker.isReadOnly())
+ 			throw new PermissionDeniedException("Database is read-only");
+ 		InputSource source;
+ 		try {
+ 			lock.acquire(Lock.WRITE_LOCK);
+ 			source = new InputSource(new StringReader(data));
+ 			XMLReader currentReader = getReader(broker);
+ 			DocumentImpl oldDoc = (DocumentImpl)documents.get(name);
+ 			DocumentImpl document = new DocumentImpl(broker, name, this);
+ 			// first pass: parse the document to determine tree structure
+ 			return determineTreeStructure(broker, name, document, oldDoc, currentReader, source);
+ 		} finally {
+ 			lock.release();
+ 		}
+ 	}
+ 	
+ 	public void store(DBBroker broker, Indexer indexer, String data, boolean privileged)
+ 	throws EXistException, PermissionDeniedException, TriggerException,
+ 	SAXException, LockException {
+ 		InputSource source = new InputSource(new StringReader(data));
+ 
+ 		// second pass: store the document
+ 		DocumentImpl document = indexer.getDocument();
+ 		LOG.debug("storing document " + document.getDocId() + " ...");
+ 		try {
+ 			try {
+ 				indexer.getReader().parse(source);
+ 			} catch (IOException e) {
+ 				throw new EXistException(e);
+ 			}
+ 
+ 			if(!hasDocument(document.getFileName())) {
+ 				addDocument(broker, document);
+ 				broker.addDocument(this, document);
+ 			} else {
+ 				broker.updateDocument(document);
+ 			}
+ 			broker.closeDocument();
+ 			broker.flush();
+ //			broker.checkTree(document);
+ 			LOG.debug("document stored.");
+ 			// if we are running in privileged mode (e.g. backup/restore)
+ 			// notify the SecurityManager about changes
+ 			if (getName().equals(SecurityManager.SYSTEM) && document.getFileName().equals(SecurityManager.ACL_FILE)
+ 			        && privileged == false) {
+ 			    // inform the security manager that system data has changed
+ 			    LOG.debug("users.xml changed");
+ 			    broker.getBrokerPool().reloadSecurityManager(broker);
+ 			}
+ 		} finally {
+ 			document.getUpdateLock().release(Lock.WRITE_LOCK);
+ 		}
+ 		broker.deleteObservers();
+ 		return;
+ 	}
+ 	
  	public DocumentImpl addDocument(DBBroker broker, String name, String data)
  			throws EXistException, PermissionDeniedException, TriggerException,
***************
*** 734,743 ****
  			lock.acquire(Lock.WRITE_LOCK);
  			source = new InputSource(new StringReader(data));
! 			document = new DocumentImpl(broker, name,	this);
  			reader = getReader(broker);
  			oldDoc = (DocumentImpl) documents.get(name);
  			
  			// first pass: parse the document to determine tree structure
! 			document = determineTreeStructure(broker, name, document, oldDoc, reader, source);
  		} finally {
  			lock.release();
--- 909,919 ----
  			lock.acquire(Lock.WRITE_LOCK);
  			source = new InputSource(new StringReader(data));
! 			document = new DocumentImpl(broker, name, this);
  			reader = getReader(broker);
  			oldDoc = (DocumentImpl) documents.get(name);
  			
  			// first pass: parse the document to determine tree structure
! 			Indexer indexer = determineTreeStructure(broker, name, document, oldDoc, reader, source);
! 			document = indexer.getDocument();
  		} finally {
  			lock.release();
***************
*** 795,799 ****
  	 * @throws TriggerException
  	 */
! 	private DocumentImpl determineTreeStructure(DBBroker broker, String name, DocumentImpl document, DocumentImpl oldDoc, XMLReader reader, InputSource source) throws LockException, EXistException, SAXException, PermissionDeniedException, TriggerException {
  		try {
  			checkPermissions(broker, name, oldDoc);
--- 971,975 ----
  	 * @throws TriggerException
  	 */
! 	private Indexer determineTreeStructure(DBBroker broker, String name, DocumentImpl document, DocumentImpl oldDoc, XMLReader reader, InputSource source) throws LockException, EXistException, SAXException, PermissionDeniedException, TriggerException {
  		try {
  			checkPermissions(broker, name, oldDoc);
***************
*** 825,829 ****
  				else {
  //					broker.checkTree(oldDoc);
! 					broker.removeDocument(getName() + '/' + oldDoc.getFileName(), false);
  				}
  				// we continue to use the old document object and just replace its contents
--- 1001,1005 ----
  				else {
  //					broker.checkTree(oldDoc);
! 					broker.removeDocument(oldDoc, false);
  				}
  				// we continue to use the old document object and just replace its contents
***************
*** 838,841 ****
--- 1014,1018 ----
  			if (trigger != null)
  				trigger.setValidating(false);
+ 			return indexer;
  		} catch(EXistException e) {
  		    if(oldDoc != null) oldDoc.getUpdateLock().release(Lock.WRITE_LOCK);
***************
*** 851,855 ****
  		    throw e;
  		}
- 		return document;
  	}
  
--- 1028,1031 ----
***************
*** 894,897 ****
--- 1070,1074 ----
  		}
  		reader.setErrorHandler(indexer);
+ 		indexer.setReader(reader);
  		//return reader;
  	}
***************
*** 933,937 ****
  	}
  
! 	/** Check Permissions about user and document, and throw exceptions if necessary.
  	 * @param broker
  	 * @param name
--- 1110,1116 ----
  	}
  
! 	/** 
! 	 * Check Permissions about user and document, and throw exceptions if necessary.
! 	 * 
  	 * @param broker
  	 * @param name
***************
*** 943,947 ****
  		if (oldDoc != null) {
  
! //			LOG.debug("Found old doc " + oldDoc.getDocId() + "; identity = " + oldDoc.hashCode());
  			// check if the document is locked by another user
  			User lockUser = oldDoc.getUserLock();
--- 1122,1127 ----
  		if (oldDoc != null) {
  
! 			LOG.debug("Found old doc " + oldDoc.getDocId() + "; identity = " + oldDoc.hashCode() +
! 					"; collection = " + hashCode());
  			// check if the document is locked by another user
  			User lockUser = oldDoc.getUserLock();
***************
*** 990,994 ****
  	
  			// first pass: parse the document to determine tree structure
! 			document = determineTreeStructure(broker, name, document, oldDoc, reader, source);
  		} finally {
  			lock.release();
--- 1170,1175 ----
  	
  			// first pass: parse the document to determine tree structure
! 			Indexer indexer = determineTreeStructure(broker, name, document, oldDoc, reader, source);
! 			document = indexer.getDocument();
  		} finally {
  			lock.release();
***************
*** 1096,1100 ****
  					broker.removeBinaryResource((BinaryDocument) oldDoc);
  				else
! 					broker.removeDocument(getName() + '/' + oldDoc.getFileName(), false);
  				oldDoc.copyOf(document);
  				indexer.setDocumentObject(oldDoc);
--- 1277,1281 ----
  					broker.removeBinaryResource((BinaryDocument) oldDoc);
  				else
! 					broker.removeDocument(oldDoc, false);
  				oldDoc.copyOf(document);
  				indexer.setDocumentObject(oldDoc);
***************
*** 1189,1192 ****
--- 1370,1374 ----
  		blob = new BinaryDocument(broker, name, this);
  		try {
+ 			lock.acquire(Lock.WRITE_LOCK);
  			checkPermissions(broker, name, oldDoc);
  
***************
*** 1198,1202 ****
  				broker.removeBinaryResource((BinaryDocument) oldDoc);
  			else
! 				broker.removeDocument(getName() + '/' + oldDoc.getFileName());
  			}
  
--- 1380,1384 ----
  				broker.removeBinaryResource((BinaryDocument) oldDoc);
  			else
! 				broker.removeDocument(oldDoc);
  			}