[jira] [Comment Edited] (JCR-3652) Bundle serialization broken

["Thomas Mueller (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/JCR-3652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13752202#comment-13752202 ] 

Thomas Mueller edited comment on JCR-3652 at 8/28/13 8:32 AM:
--------------------------------------------------------------

A simpler test case:

{code}
Node node = root.addNode(nodeName);
superuser.save();
try {
    node.setProperty("jcr:isCheckedOut", true);
} catch (ArrayIndexOutOfBoundsException e) {
    System.out.println("set failed strangely: " + e);
}
System.out.println(node.getProperty("jcr:isCheckedOut").getValue().getString());
{code}

I think what happens is: the property is half-way added (in NodeImpl.SetPropertyOperation.perform, PropertyImpl property = getOrCreateProperty(name, type, false, enforceType, status)). The property is non-multi-valued, and the values array is 0. This seems to be always done like that. Then it tries to set the value, with 

{code}
try { 
  property.setValue(value); 
} catch(RepositoryException) { 
  ... revert ... 
}
{code}

The setValue checks if setting the value is allowed (via checkSetValue, which calls ItemValidator.checkModify, which calls ItemValidator.checkCondition). At some point this verification calls NodeImpl.isCheckedOut(). There is a "// FIXME should not only rely on existence of jcr:isCheckedOut property" in the code, but for performance reason it just gets the value of the jcr:isCheckedOut property, using "return ps.getValues()[0].getBoolean();". Now, as the property is half-way set, this throws an ArrayOutOfBoundsException (half-way set properties have an empty values array). Because this isn't a RepositoryException, this is thrown back to the user, and the property remains half-way set. The internal state of the node is corrupt.

One solution is to change NodeImpl.isCheckedOut() to deal with a half-way set property, and throw a RepositoryException instead of a ArrayOutOfBoundsException. However, there might be more checks that don't deal with half-way set properties.

In addition to the above, instead of just catching RepositoryException, the code could catch Exception or even Throwable (that is, also Error). Usually catching Throwable isn't a good idea, however the alternative (having a corrupt node) is much worse, so I guess this should also be done.

The best solution would be not to have half-way set properties at all, but that would be quite a big change, so I will not try to do that.

                
      was (Author: tmueller):
    A simpler test case:

{code}
Node node = root.addNode(nodeName);
superuser.save();
try {
    node.setProperty("jcr:isCheckedOut", true);
} catch (ArrayIndexOutOfBoundsException e) {
    System.out.println("set failed strangely: " + e);
}
System.out.println(node.getProperty("jcr:isCheckedOut").getValue().getString());
{code}

I think what happens is: the property is half-way added (in NodeImpl.SetPropertyOperation.perform, PropertyImpl property = getOrCreateProperty(name, type, false, enforceType, status);). The property is non-multi-valued, and the values array is 0. This seems to be always done like that. Then it tries to set the value, with a try { property.setValue(value); } catch(RepositoryException) { ... revert ... }. The setValue checks if setting the value is allowed (via checkSetValue, which calls ItemValidator.checkModify, which calls ItemValidator.checkCondition). At some point this verification calls NodeImpl.isCheckedOut(). There is a "// FIXME should not only rely on existence of jcr:isCheckedOut property" in the code, but for performance reason it just gets the value of the jcr:isCheckedOut property, using "return ps.getValues()[0].getBoolean();". Now, as the property is half-way set, this throws an ArrayOutOfBoundsException (half-way set properties have an empty values array). Because this isn't a RepositoryException, this is thrown back to the user, and the property remains half-way set. The internal state of the node is corrupt.

One solution is to change NodeImpl.isCheckedOut() to deal with a half-way set property, and throw a RepositoryException instead of a ArrayOutOfBoundsException. However, there might be more checks that don't deal with half-way set properties.

In addition to the above, instead of just catching RepositoryException, the code could catch Exception or even Throwable (that is, also Error). Usually catching Throwable isn't a good idea, however the alternative (having a corrupt node) is much worse, so I guess this should also be done.

The best solution would be not to have half-way set properties at all, but that would be quite a big change, so I will not try to do that.

                  
> Bundle serialization broken
> ---------------------------
>
>                 Key: JCR-3652
>                 URL: https://issues.apache.org/jira/browse/JCR-3652
>             Project: Jackrabbit Content Repository
>          Issue Type: New Feature
>          Components: jackrabbit-core
>            Reporter: Thomas Mueller
>            Assignee: Thomas Mueller
>            Priority: Minor
>             Fix For: 2.4.4, 2.7.1
>
>         Attachments: JCR-3652.patch, JCR-3652-test-case.patch
>
>
> I have got a strange case where some node bundle is broken, seemingly because a byte is missing. I can't explain the missing byte, but it is reproducible, meaning that writing the bundles again will break them again. There are 11 broken bundles, 10 of them have the size 480 bytes and one is slightly larger. It is always a boolean property value that is missing, always the value for the property jcr:isCheckedOut.
> As a (temporary) solution, and to help analyze what the problem might be, I will create a patch that does the following:
> * When serializing a bundle, check if the byte array can be de-serialized. If not, then try again. Starting with the 3th try, use a slower variant where before and after writing the boolean value the buffer is flushed. I'm aware that ByteArrayOutputStream.flush doesn't do much, but maybe it solves the problem (let's see) if the problem is related to a JVM issue.
> * If de-serializing a bundle fails, check if it's because of a missing boolean property value. If yes, insert the missing byte.
> I have also added some log messages (warning / error) to help analyze the problem.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira