Index: src/main/java/org/apache/jackrabbit/core/PropertyImpl.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/PropertyImpl.java (revision 801555) +++ 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 801555) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBFileValue.java (working copy) @@ -18,13 +18,12 @@ import java.io.IOException; import java.io.InputStream; -import java.io.ByteArrayOutputStream; -import java.io.UnsupportedEncodingException; import javax.jcr.RepositoryException; import javax.jcr.Binary; import org.apache.jackrabbit.core.data.DataIdentifier; +import org.apache.commons.io.IOUtils; /** * Represents binary data which is backed by a resource or byte[]. @@ -42,32 +41,18 @@ * Returns a String representation of this value. * * @return String representation of this value. - * @throws RepositoryException + * @throws RepositoryException if an error occurs while reading from this + * binary. */ 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); + return IOUtils.toString(stream, "UTF-8"); } catch (IOException e) { throw new RepositoryException("conversion from stream to string failed", e); } finally { - try { - if (stream != null) { - stream.close(); - } - } catch (IOException e) { - // ignore - } + IOUtils.closeQuietly(stream); } } @@ -89,12 +74,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 +103,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 { Index: src/main/java/org/apache/jackrabbit/core/value/BLOBInDataStore.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBInDataStore.java (revision 801555) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInDataStore.java (working copy) @@ -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 801555) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInMemory.java (working copy) @@ -128,8 +128,8 @@ // the data will be garbage collected } - boolean isImmutable() { - return true; + BLOBFileValue copy() throws RepositoryException { + return this; } public long getSize() { Index: src/main/java/org/apache/jackrabbit/core/value/BLOBInResource.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/BLOBInResource.java (revision 801555) +++ src/main/java/org/apache/jackrabbit/core/value/BLOBInResource.java (working copy) @@ -96,9 +96,8 @@ // 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 801555) +++ 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); } @@ -117,9 +121,12 @@ } } - 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 801555) +++ src/main/java/org/apache/jackrabbit/core/value/InternalValue.java (working copy) @@ -115,7 +115,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 +130,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 +347,7 @@ * @throws RepositoryException */ public static InternalValue create(InputStream value) throws RepositoryException { - return new InternalValue(getBLOBFileValue(null, value, false)); + return create(value, null); } /** @@ -432,21 +445,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()); } /** @@ -679,7 +679,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 Index: src/main/java/org/apache/jackrabbit/core/value/InternalValueFactory.java =================================================================== --- src/main/java/org/apache/jackrabbit/core/value/InternalValueFactory.java (revision 801555) +++ 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 discarded = 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. + */ + void discard() { + if (refCount.get() > 0) { + if (refCount.decrementAndGet() == 0) { + log.debug("{}@refCount={}, discarding value...", + System.identityHashCode(this), refCount); + value.discard(); + discarded = 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 (!discarded) { + discard(); + } + 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 discard() { + 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 801555) +++ 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 801555) +++ 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);