Index: src/main/java/org/apache/jackrabbit/core/PropertyImpl.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/PropertyImpl.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/PropertyImpl.java (working copy) @@ -17,6 +17,7 @@ package org.apache.jackrabbit.core; import java.io.InputStream; +import java.io.IOException; import java.math.BigDecimal; import java.util.ArrayList; import java.util.Calendar; @@ -49,6 +50,7 @@ import org.apache.jackrabbit.spi.commons.name.NameConstants; import org.apache.jackrabbit.spi.commons.value.ValueFormat; import org.apache.jackrabbit.value.ValueHelper; +import org.apache.commons.io.input.AutoCloseInputStream; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -474,7 +476,14 @@ } public InputStream getStream() throws RepositoryException { - return getValue().getBinary().getStream(); + final Binary bin = getValue().getBinary(); + // make sure binary is disposed after stream had been consumed + return new AutoCloseInputStream(bin.getStream()) { + public void close() throws IOException { + super.close(); + bin.dispose(); + } + }; } public long getLong() throws RepositoryException { Index: src/main/java/org/apache/jackrabbit/core/value/BLOBFileValue.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBFileValue.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBFileValue.java (working copy) @@ -18,8 +18,6 @@ import java.io.IOException; import java.io.InputStream; -import java.io.ByteArrayOutputStream; -import java.io.UnsupportedEncodingException; import javax.jcr.RepositoryException; import javax.jcr.Binary; @@ -39,48 +37,6 @@ abstract class BLOBFileValue implements Binary { /** - * Returns a String representation of this value. - * - * @return String representation of this value. - * @throws RepositoryException - */ - String getString() throws RepositoryException { - // TODO: review again. currently the getString method of the JCR Value is delegated to the QValue. - InputStream stream = getStream(); - try { - ByteArrayOutputStream out = new ByteArrayOutputStream(); - byte[] buffer = new byte[8192]; - int read; - while ((read = stream.read(buffer)) > 0) { - out.write(buffer, 0, read); - } - byte[] data = out.toByteArray(); - return new String(data, "UTF-8"); - } catch (UnsupportedEncodingException e) { - throw new RepositoryException("UTF-8 not supported on this platform", e); - } catch (IOException e) { - throw new RepositoryException("conversion from stream to string failed", e); - } finally { - try { - if (stream != null) { - stream.close(); - } - } catch (IOException e) { - // ignore - } - } - } - - /** - * Frees temporarily allocated resources such as temporary file, buffer, etc. - * If this BLOBFileValue is backed by a persistent resource - * calling this method will have no effect. - * - * @see #delete(boolean) - */ - abstract void discard(); - - /** * Deletes the persistent resource backing this BLOBFileValue. * * @param pruneEmptyParentDirs if true, empty parent directories @@ -89,12 +45,18 @@ abstract void delete(boolean pruneEmptyParentDirs); /** - * Checks if this object is immutable. - * Immutable objects can not change and can safely copied. + * Returns a copy of this BLOB file value. The returned copy may also be + * this object. However an implementation must guarantee that the returned + * value has state that is independent from this value. Immutable values + * can savely return the same value (this object). + *

+ * Specifically, {@link #dispose()} on the returned value must not have an + * effect on this value! * - * @return true if the object is immutable + * @return a value that can be used independently from this value. + * @throws RepositoryException if an error occur while copying this value. */ - abstract boolean isImmutable(); + abstract BLOBFileValue copy() throws RepositoryException; public abstract boolean equals(Object obj); @@ -112,10 +74,7 @@ } //-----------------------------------------------------< javax.jcr.Binary > - public abstract long getSize(); - public abstract InputStream getStream() throws RepositoryException; - public int read(byte[] b, long position) throws IOException, RepositoryException { InputStream in = getStream(); try { @@ -125,9 +84,4 @@ in.close(); } } - - public void dispose() { - discard(); - } - } Index: src/main/java/org/apache/jackrabbit/core/value/BLOBInDataStore.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBInDataStore.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInDataStore.java (working copy) @@ -57,7 +57,7 @@ // do nothing } - void discard() { + public void dispose() { // do nothing } @@ -65,8 +65,8 @@ return identifier; } - boolean isImmutable() { - return true; + BLOBFileValue copy() throws RepositoryException { + return this; } public boolean equals(Object obj) { Index: src/main/java/org/apache/jackrabbit/core/value/BLOBInMemory.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBInMemory.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInMemory.java (working copy) @@ -22,7 +22,6 @@ import javax.jcr.RepositoryException; import java.io.ByteArrayInputStream; import java.io.InputStream; -import java.io.UnsupportedEncodingException; import java.util.Arrays; /** @@ -122,14 +121,14 @@ // the data will be garbage collected } - void discard() { + public void dispose() { // do nothing // this object could still be referenced // the data will be garbage collected } - boolean isImmutable() { - return true; + BLOBFileValue copy() throws RepositoryException { + return this; } public long getSize() { @@ -140,14 +139,6 @@ return new ByteArrayInputStream(data); } - String getString() throws RepositoryException { - try { - return new String(data, "UTF-8"); - } catch (UnsupportedEncodingException e) { - throw new RepositoryException("UTF-8 not supported on this platform", e); - } - } - public String toString() { StringBuilder buff = new StringBuilder(PREFIX.length() + 2 * data.length); buff.append(PREFIX); Index: src/main/java/org/apache/jackrabbit/core/value/BLOBInResource.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBInResource.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInResource.java (working copy) @@ -92,13 +92,12 @@ } - void discard() { + public void dispose() { // this instance is not backed by temporarily allocated resource/buffer } - boolean isImmutable() { - // delete will modify the state. - return false; + BLOBFileValue copy() throws RepositoryException { + return BLOBInTempFile.getInstance(getStream(), true); } public long getSize() { Index: src/main/java/org/apache/jackrabbit/core/value/BLOBInTempFile.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBInTempFile.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInTempFile.java (working copy) @@ -92,8 +92,12 @@ * @param in the stream * @param temp */ - static BLOBInTempFile getInstance(InputStream in, boolean temp) throws RepositoryException { - return new BLOBInTempFile(in, temp); + static BLOBFileValue getInstance(InputStream in, boolean temp) throws RepositoryException { + if (temp) { + return new RefCountingBLOBFileValue(new BLOBInTempFile(in, temp)); + } else { + return new BLOBInTempFile(in, temp); + } } /** @@ -101,7 +105,7 @@ * * @param file the file */ - static BLOBInTempFile getInstance(File file, boolean temp) throws IOException { + static BLOBInTempFile getInstance(File file, boolean temp) { return new BLOBInTempFile(file, temp); } @@ -111,15 +115,18 @@ file = null; } - void discard() { + public void dispose() { if (temp) { delete(true); } } - boolean isImmutable() { - // discard and delete can modify the state. - return false; + BLOBFileValue copy() throws RepositoryException { + if (temp) { + return BLOBInTempFile.getInstance(getStream(), temp); + } else { + return BLOBInTempFile.getInstance(file, temp); + } } public long getSize() { Index: src/main/java/org/apache/jackrabbit/core/value/InternalValue.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/InternalValue.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/InternalValue.java (working copy) @@ -49,6 +49,7 @@ import org.apache.jackrabbit.spi.commons.value.AbstractQValueFactory; import org.apache.jackrabbit.spi.commons.value.QValueValue; import org.apache.jackrabbit.util.ISO8601; +import org.apache.commons.io.IOUtils; /** * InternalValue represents the internal format of a property value. @@ -115,7 +116,6 @@ throws ValueFormatException, RepositoryException { switch (value.getType()) { case PropertyType.BINARY: - InternalValue result; BLOBFileValue blob = null; if (value instanceof BinaryValueImpl) { BinaryValueImpl bin = (BinaryValueImpl) value; @@ -131,10 +131,24 @@ } } if (blob == null) { - blob = getBLOBFileValue(store, value.getBinary().getStream(), true); + Binary b = value.getBinary(); + boolean dispose = false; + try { + if (b instanceof BLOBFileValue) { + // use as is + blob = (BLOBFileValue) b; + } else { + // create a copy from the stream + dispose = true; + blob = getBLOBFileValue(store, b.getStream(), true); + } + } finally { + if (dispose) { + b.dispose(); + } + } } - result = new InternalValue(blob); - return result; + return new InternalValue(blob); case PropertyType.BOOLEAN: return create(value.getBoolean()); case PropertyType.DATE: @@ -334,7 +348,7 @@ * @throws RepositoryException */ public static InternalValue create(InputStream value) throws RepositoryException { - return new InternalValue(getBLOBFileValue(null, value, false)); + return create(value, null); } /** @@ -432,21 +446,8 @@ // wrapped value is immutable (and therefore this instance as well) return this; } - BLOBFileValue v = (BLOBFileValue) val; - if (v.isImmutable()) { - return this; - } - // return a copy since the wrapped BLOBFileValue instance is mutable - InputStream stream = v.getStream(); - try { - return createTemporary(stream); - } finally { - try { - stream.close(); - } catch (IOException e) { - // ignore - } - } + // return a copy of the wrapped BLOBFileValue + return new InternalValue(((BLOBFileValue) val).copy()); } /** @@ -639,7 +640,7 @@ */ public long getLength() throws RepositoryException { if (PropertyType.BINARY == type) { - return ((BLOBFileValue) val).getSize(); + return ((Binary) val).getSize(); } else { return super.getLength(); } @@ -650,7 +651,14 @@ */ public String getString() throws RepositoryException { if (type == PropertyType.BINARY) { - return ((BLOBFileValue) val).getString(); + InputStream stream = getStream(); + try { + return IOUtils.toString(stream, "UTF-8"); + } catch (IOException e) { + throw new RepositoryException("conversion from stream to string failed", e); + } finally { + IOUtils.closeQuietly(stream); + } } else if (type == PropertyType.DATE) { return ISO8601.format(((Calendar) val)); } else { @@ -663,7 +671,7 @@ */ public InputStream getStream() throws RepositoryException { if (type == PropertyType.BINARY) { - return ((BLOBFileValue) val).getStream(); + return ((Binary) val).getStream(); } else { try { // convert via string @@ -679,7 +687,9 @@ */ public Binary getBinary() throws RepositoryException { if (type == PropertyType.BINARY) { - return (BLOBFileValue) val; + // return an independent copy that can be disposed without + // affecting this value + return ((BLOBFileValue) val).copy(); } else { try { // convert via string @@ -697,7 +707,7 @@ public void discard() { if (type == PropertyType.BINARY) { BLOBFileValue bfv = (BLOBFileValue) val; - bfv.discard(); + bfv.dispose(); } else { super.discard(); } Index: src/main/java/org/apache/jackrabbit/core/value/InternalValueFactory.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/InternalValueFactory.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/InternalValueFactory.java (working copy) @@ -150,7 +150,11 @@ } public QValue create(InputStream value) throws RepositoryException, IOException { - return InternalValue.create(value, store); + if (store == null) { + return InternalValue.createTemporary(value); + } else { + return InternalValue.create(value, store); + } } public QValue create(File value) throws RepositoryException, IOException { Index: src/main/java/org/apache/jackrabbit/core/value/RefCountingBLOBFileValue.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/RefCountingBLOBFileValue.java (revision 0) +++ src/main/java/org/apache/jackrabbit/core/value/RefCountingBLOBFileValue.java (revision 0) @@ -0,0 +1,210 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.core.value; + +import java.io.InputStream; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.jcr.RepositoryException; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * RefCountingBLOBFileValue implements a reference counting BLOB + * file value on top of an existing {@link BLOBFileValue}. Whenever a + * {@link #copy()} is created from this BLOB file value, a new light weight + * {@link RefCountBinary} is created and the reference count {@link #refCount} + * is incremented. The underlying value is discarded once this + * {@link RefCountingBLOBFileValue} and all its light weight + * {@link RefCountBinary} intances have been discarded. + */ +public class RefCountingBLOBFileValue extends BLOBFileValue { + + /** + * The logger instance for this class. + */ + private static final Logger log = LoggerFactory.getLogger(RefCountingBLOBFileValue.class); + + /** + * The actual value. + */ + private final BLOBFileValue value; + + /** + * The current ref count. Initially set to one. + */ + private final AtomicInteger refCount = new AtomicInteger(1); + + /** + * Whether this instance has been discarded and cannot be used anymore. + */ + private boolean disposed = false; + + /** + * Creates a new reference counting blob file value based on the given + * value. + * + * @param value the underlying value. + */ + public RefCountingBLOBFileValue(BLOBFileValue value) { + this.value = value; + } + + //----------------------------< BLOBFileValue >----------------------------- + + /** + * Discards the underyling value if the reference count drops to zero. + */ + public void dispose() { + if (refCount.get() > 0) { + if (refCount.decrementAndGet() == 0) { + log.debug("{}@refCount={}, discarding value...", + System.identityHashCode(this), refCount); + value.dispose(); + disposed = true; + } else { + log.debug("{}@refCount={}", + System.identityHashCode(this), refCount); + } + } + } + + /** + * Forwards the call to the underlying value. + * + * @param pruneEmptyParentDirs if true, empty parent + * directories will automatically be deleted + */ + void delete(boolean pruneEmptyParentDirs) { + value.delete(pruneEmptyParentDirs); + } + + /** + * Returns a light weight copy of this BLOB file value. + * + * @return a copy of this value. + * @throws RepositoryException if an error occurs while creating the copy or + * if this value has been disposed already. + */ + BLOBFileValue copy() throws RepositoryException { + if (refCount.get() <= 0) { + throw new RepositoryException("this BLOBFileValue has been disposed"); + } + BLOBFileValue bin = new RefCountBinary(); + refCount.incrementAndGet(); + log.debug("{}@refCount={}", System.identityHashCode(this), refCount); + return bin; + } + + public boolean equals(Object obj) { + if (obj instanceof RefCountingBLOBFileValue) { + RefCountingBLOBFileValue val = (RefCountingBLOBFileValue) obj; + return value.equals(val.value); + } + return false; + } + + public String toString() { + return value.toString(); + } + + public int hashCode() { + return 0; + } + + //-----------------------------------------------------< javax.jcr.Binary > + + public long getSize() throws RepositoryException { + return value.getSize(); + } + + public InputStream getStream() throws RepositoryException { + return value.getStream(); + } + + protected void finalize() throws Throwable { + if (!disposed) { + dispose(); + } + super.finalize(); + } + + //------------------------< RefCountBinary >-------------------------------- + + private final class RefCountBinary extends BLOBFileValue { + + private boolean disposed; + + public InputStream getStream() throws RepositoryException { + checkDisposed(); + return getInternalValue().getStream(); + } + + public long getSize() throws RepositoryException { + checkDisposed(); + return getInternalValue().getSize(); + } + + public void dispose() { + if (!disposed) { + disposed = true; + getInternalValue().dispose(); + } + } + + void delete(boolean pruneEmptyParentDirs) { + getInternalValue().delete(pruneEmptyParentDirs); + } + + BLOBFileValue copy() throws RepositoryException { + checkDisposed(); + return getInternalValue().copy(); + } + + public boolean equals(Object obj) { + if (obj instanceof RefCountBinary) { + RefCountBinary other = (RefCountBinary) obj; + return getInternalValue().equals(other.getInternalValue()); + } + return false; + } + + public String toString() { + return getInternalValue().toString(); + } + + public int hashCode() { + return 0; + } + + protected void finalize() throws Throwable { + dispose(); + super.finalize(); + } + + private BLOBFileValue getInternalValue() { + return RefCountingBLOBFileValue.this; + } + + private void checkDisposed() throws RepositoryException { + if (disposed) { + throw new RepositoryException("this BLOBFileValue is disposed"); + } + } + } +} Property changes on: src\main\java\org\apache\jackrabbit\core\value\RefCountingBLOBFileValue.java ___________________________________________________________________ Added: svn:eol-style + native Index: src/main/java/org/apache/jackrabbit/core/value/ValueFactoryImpl.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/ValueFactoryImpl.java (revision 801640) +++ src/main/java/org/apache/jackrabbit/core/value/ValueFactoryImpl.java (working copy) @@ -21,6 +21,7 @@ import org.apache.jackrabbit.spi.commons.conversion.NamePathResolver; import org.apache.jackrabbit.spi.commons.value.ValueFactoryQImpl; import org.apache.jackrabbit.spi.QValue; +import org.apache.jackrabbit.value.BinaryImpl; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -59,7 +60,7 @@ public Value createValue(QValue qvalue) { if (qvalue instanceof InternalValue && PropertyType.BINARY == qvalue.getType()) { try { - return new BinaryValueImpl(((InternalValue) qvalue).getBLOBFileValue()); + return new BinaryValueImpl(((InternalValue) qvalue).getBLOBFileValue().copy()); } catch (RepositoryException e) { // should not get here log.error(e.getMessage(), e); @@ -68,6 +69,19 @@ return super.createValue(qvalue); } + public Binary createBinary(InputStream stream) throws RepositoryException { + try { + QValue value = getQValueFactory().create(stream); + if (value instanceof InternalValue) { + return ((InternalValue) value).getBLOBFileValue(); + } else { + return new BinaryImpl(stream); + } + } catch (IOException e) { + throw new RepositoryException(e); + } + } + public Value createValue(Binary binary) { try { if (binary instanceof BLOBInDataStore) { @@ -77,6 +91,8 @@ // if the value is already in this data store return new BinaryValueImpl(value.getBLOBFileValue()); } + } else if (binary instanceof BLOBFileValue) { + return new BinaryValueImpl(((BLOBFileValue) binary).copy()); } return createValue(binary.getStream()); } catch (RepositoryException e) { Index: src/test/java/org/apache/jackrabbit/core/value/BinaryValueTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/core/value/BinaryValueTest.java (revision 0) +++ src/test/java/org/apache/jackrabbit/core/value/BinaryValueTest.java (revision 0) @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.core.value; + +import java.io.ByteArrayInputStream; +import java.util.Random; + +import javax.jcr.RepositoryException; +import javax.jcr.Property; +import javax.jcr.Binary; +import javax.jcr.Node; + +import org.apache.jackrabbit.test.AbstractJCRTest; + +/** + * BinaryValueTest check if multiple execution of: + *

+ * do not throw an exception. + *

+ * See also JCR-2238. + */ +public class BinaryValueTest extends AbstractJCRTest { + + public void testDispose10() throws Exception { + checkDispose(10, false); + } + + public void testDispose10k() throws Exception { + checkDispose(10 * 1024, false); + } + + public void testDispose10Save() throws Exception { + checkDispose(10, true); + } + + public void testDispose10kSave() throws Exception { + checkDispose(10 * 1024, true); + } + + protected void checkDispose(int length, boolean save) throws Exception { + Property prop = setProperty(testRootNode.addNode(nodeName1), length); + if (save) { + superuser.save(); + } + checkProperty(prop); + } + + protected Property setProperty(Node node, int length) throws RepositoryException { + Random rand = new Random(); + byte[] data = new byte[length]; + rand.nextBytes(data); + + Binary b = vf.createBinary(new ByteArrayInputStream(data)); + System.out.println(b.getClass() + ": " + System.identityHashCode(b)); + try { + return node.setProperty(propertyName1, b); + } finally { + b.dispose(); + } + } + + protected void checkProperty(Property prop) throws Exception { + for (int i = 0; i < 3; i++) { + Binary b = prop.getBinary(); + try { + //System.out.println(b.getClass() + ": " + System.identityHashCode(b)); + b.read(new byte[1], 0); + } finally { + b.dispose(); + } + } + } +} Property changes on: src\test\java\org\apache\jackrabbit\core\value\BinaryValueTest.java ___________________________________________________________________ Added: svn:eol-style + native Index: src/test/java/org/apache/jackrabbit/core/value/TestAll.java =================================================================== --- src/test/java/org/apache/jackrabbit/core/value/TestAll.java (revision 801640) +++ src/test/java/org/apache/jackrabbit/core/value/TestAll.java (working copy) @@ -32,6 +32,7 @@ public static Test suite() { TestSuite suite = new TestSuite("org.apache.jackrabbit.core.value tests"); + suite.addTestSuite(BinaryValueTest.class); suite.addTestSuite(InternalValueFactoryTest.class); suite.addTestSuite(InternalValueTest.class);