Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false

• Key: JCR-1388
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Esteban Franqueiro
• Component/s: jackrabbit-core
• Affects Version/s: 1.4
• Fix Version/s: 1.5.0

Description

Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
See JCR-1184 for a test for this problem (run it with copyWhenReading=false).

Comments

[Esteban Franqueiro]

This patch addresses the issue by provinding a wrapper input stream that encapsulates the database resources, and not returning the connection to the pool until the stream is consumed/closed.

[Esteban Franqueiro]

Hi.
Did anyone take a look at this patch?
Regards

[Thomas Mueller]

Hi,

I have a few remarks, first about the source code 'style'. I use Eclipse and the Checkstyle plugin, this should find most issues:
- you should use spaces instead of tabs
- return doesn't required brackets: return(false) should be changed to return false
- catch (IOException e) {} should at least have a remark, but it's better to log the exception (with stack trace)
- you need to replace the file file headers
- use '} else {', '} finally {' and '} catch (Exception e) {' as in the Sun Java coding guidelines
- Don't declare all variables at the start of the method as in C. Declare them when / just before using them
  (for example, getResourceAsReader(), reader; there are others
- Review the Javadocs rules (add comments, use the @param, @return tags)
- Only use this. when required

Some other remarks:
- I didn't see any test cases - please add one
- Are prepareSchemaObjectPrefix and getResourceAsReader used somewhere? Don't add unused methods
- close() methods easting exceptions should be called closeSilently()
- You have removed the SQL statement remark, why? // SELECT ID, DATA FROM DATASTORE WHERE ID = ?
- If you are removing code, remove the lines, don't remark them (+//lastModified = ...)
- getDatabaseResources, boolean success is always true
- You hare remarked "lastModified = store.touch(getIdentifier(), lastModified)", why?
- Don't swallow exceptions (use IOException.initCause in DbInputStream.getStream())
- Synchronization is very inconsistent (DbInputStream)

Regards,
Thomas

[Esteban Franqueiro]

Hi.

> I have a few remarks, first about the source code 'style'. I use Eclipse and the Checkstyle plugin, this should find most issues:
> - you should use spaces instead of tabs
> - return doesn't required brackets: return(false) should be changed to return false
> - catch (IOException e) {} should at least have a remark, but it's better to log the exception (with stack trace)
> - you need to replace the file file headers
> - use '} else {', '} finally {' and '} catch (Exception e) {' as in the Sun Java coding guidelines
> - Don't declare all variables at the start of the method as in C. Declare them when / just before using them
> (for example, getResourceAsReader(), reader; there are others
> - Review the Javadocs rules (add comments, use the @param, @return tags)
> - Only use this. when required

Fixed all of those I think.

> Some other remarks:
> - I didn't see any test cases - please add one
> - Are prepareSchemaObjectPrefix and getResourceAsReader used somewhere? Don't add unused methods

Removed them.

> - close() methods easting exceptions should be called closeSilently()

Renamed them.

> - You have removed the SQL statement remark, why? // SELECT ID, DATA FROM DATASTORE WHERE ID = ?

Because I deleted the entire method.

> - If you are removing code, remove the lines, don't remark them (+//lastModified = ...)
> - getDatabaseResources, boolean success is always true
> - You hare remarked "lastModified = store.touch(getIdentifier(), lastModified)", why?

Fixed those.

> - Don't swallow exceptions (use IOException.initCause in DbInputStream.getStream())
> - Synchronization is very inconsistent (DbInputStream)

In what sense? The DbInputStream is not supposed to be used from multiple threads, so I only synchronized the methods that are already synchronized in InputStream.

I'll attach the new patch and test.

Regards.

[Esteban Franqueiro]

The new patch corrects the issues pointed by Thomas. The test passes with the patch applied, copyWhenReading=false and storeStream=-1. Without the patch, the test fails with the same conditions.

[Thomas Mueller]

Hi,

Your new patch is better, but there are still a few issues:

You still have tabs in the source code.

DbResources and DatabaseHelper still don't have correct license headers.

There is still a catch (Exception e) {}.

There are still some cases where "} else {" and so on is not on the same line. Please format the source code. In Eclipse, use [Source] [Format]. I suggest you review the Sun coding guidelines at http://java.sun.com/docs/codeconv/

Javadoc comments are still not correct. getLastModified doesn't have a Javadoc comment. @return tag is still not used.

Variables are still declared at the start of the method in getDatabaseResources(). Please declare them when / just before using them

DbInputStream.mark and reset are synchronized, but nothing else in this class, why?

The SQL statement remark "// SELECT ID, DATA FROM DATASTORE WHERE ID = ?" was
there to simplify reading the code (so you don't have to switch to another file to understand it).
Please add it where selectDataSQL is used.

About the method finalize(): I wouldn't use it. It slows down creating objects. If the wrapped InputStream and resource need finalize(), it is already implemented there.

About the test cases:

- ".classpath" doesn't exist in all systems (RandomInputStream is better).

- Don't use System.out.println, use a logger.

- Add the test to the TestAll method.

- You have again used return(..) instead of return ..

- The Javadoc comment is empty

Regards,
Thomas

[Esteban Franqueiro]

Ok, here's a new take at it. I hope I resolved all the issues you pointed out.

> DbInputStream.mark and reset are synchronized, but nothing else in this class, why?

I followed the pattern used in InputStream and FilterInputStream.

Regarding the finalizer, I removed it, but I think that it depends on what your position is regarding improper use of the stream. If the policy is to protect against that kind of mistake or to let it happen so that in the end the user has to fix it's application.

[Thomas Mueller]

Hi,

Your new patch is better, but there are still a few issues:

You still have tabs in the source code: 2 to go ;-)

There is still a catch (Exception e) {}. catch (IOException e) {} should at least have a remark, but it's better to log the exception (with stack trace)

TestTwoGetStreams.streamToString() expects to read everything with one InputStreamReader.read call. This method could theoretically return a value smaller than requested in length. You should deal with that in some way; for example, use DataInputStream.readFully. Also, the conversion to String expects the data is stored in the current system encoding, that may not be the case.

I wouldn't use new FileInputStream("NOTICE.txt"), I would use new RandomInputStream(0, 10 * 1024 * 1024). You can't be sure that NOTICE.txt will always be there and have that name.

The method testTwoGetStreams doesn't have any assertion. It doesn't check if the data read from the repository is the same as in the files. Only calling log.info is not a good test I believe.

DatabaseHelper doesn't have a class level javadoc.

getDatabaseResources: is the 'finally' and the boolean success required? Could the code in 'finally' not be written in the 'catch' block?

DbInputStream.mark and reset are synchronized: I don't understand why this needs to be synchronized in your class as well. I probably need to ask Arthur van Hoff then ;-)

Regards,
Thomas

[Esteban Franqueiro]

Here's a new version. I removed the synchronization from DbInputStream. I'm also posting a new version of the test, and the previous, fixed, version in the patch.
Regards

[Thomas Mueller]

Hi,

In TestTwoGetStreams there are many 'if' without {}:

            if (i2 != null)
                i2.close();

According to the code style, this should be written

            if (i2 != null) {
                i2.close();
            }

As I wrote before, I suggest you use Checkstyle so that you can find such problems yourself.

What is the reason for the following code?

        i1.close();
        i2.close();
        try {
            if (i1 != null)
                i1.close();
        } catch (IOException e) {
            log.info("Could not close first input stream: ", e);
        }
        try {
            if (i2 != null)
                i2.close();
        } catch (IOException e) {
            log.info("Could not close second input stream: ", e);
        }

Why do you close i1 and i2 twice? Why do you check for null?

The line
private final static int BLOCK_SIZE = 4 * 1024;
should be (according to Checkstyle):
private static final int BLOCK_SIZE = 4 * 1024;

Your method assertEquals is incorrect:

            while (n1 != -1 || n2 != -1) {
                n1 = i1.read(b1);
                n2 = i2.read(b2);
                for (int i = 0; i < n1; i++) {
                    assertEquals(message + "; byte #" + i + " mismatch!", b2[i], b1[i]);
                }
                count1 += n1;
                count2 += n2;
            }

i1 and i2 may not return the same number of bytes (n1 != n2). I suggest to test byte by byte.

Otherwise everything looks very good to me.

Regards,
Thomas

[Esteban Franqueiro]

> As I wrote before, I suggest you use Checkstyle so that you can find such problems yourself.

I'm using it, but I missed those.

> What is the reason for the following code?
> ...
> Why do you close i1 and i2 twice? Why do you check for null?

Oops my mistake. Fixed that. I left the try/catch calls, so that it logs the exception. Should I re-throw it?

[Esteban Franqueiro]

Oops, wrong file. Here is the right one.

[Thomas Mueller]

Hi,
Sorry for the delay... I think there is one potential problem in assertEquals(String message, InputStream i1, InputStream i2):

+ int b1 = 0, b2 = 0;
+ int i = 0;
+ while (b1 != -1 || b2 != -1) {
+ b1 = i1.read();
+ b2 = i2.read();
+ assertEquals(message + "; byte #" + i + " mismatch!", (byte) b2, (byte) b1);
+ ++i;
+ }

This test wouldn't detect a problem if b1 is -1 and b2 is 255. I would change the code to:

assertEquals(message + "; byte #" + i + " mismatch!", b2, b1);

Everything else looks fine to me.
Regards,
Thomas

[Esteban Franqueiro]

I agree with you Thomas. Here's the corrected patch.

[Thomas Mueller]

+1
The patch looks good to me.

[Thomas Mueller]

Committed in revision 649493 (trunk)