#109 CSVReader mishandles BufferedReader, cutting off input

[Stephen A. Goss]

The CSVReader falsely assumes that the BufferedReader is closed when it is not ready. If the slowness of the input IO feeding the buffer is slower than the parsing itself, the buffer might not be ready, which opencsv considers "closed" and stops parsing erroneously.

    /**

     * Checks to see if the file is closed.
     * @return true if the reader can no longer be read from.
     */
    private boolean isClosed() {
        if (!verifyReader) {
            return false;
        }
        try {
            return !br.ready(); // WRONG!!! buffer might just be waiting
        } catch (IOException e) {
            return true;
        }
    }

[Stephen A. Goss]

I see this was already reported with issue 108, but there is actually no case where you'd want this behavior. Normal testing doesn't reveal when a buffer might drain due to slow IO. I only found this in integration testing, where on a slower network, my CSVs were coming up short. It had nothing to do with Channels either.

[Scott Conway]

Hello Stephen - this seems to be the most contentious change in opencsv 3.0
<bg>. Personally though I never thought people would be streaming
information over a network - you must be working on some pretty cool stuff.</bg>

But as for the last statement in your email there was a very good reason
for it and that was Groovy Lambdas (and quite possibly Java 8 Lambdas).

Check out the original bug at stack overflow
http://stackoverflow.com/questions/23058244/csvparser-stream-closed-error-when-parsing-multiple-files/23058601#23058601

The issue seems to be because of lazy evaluation Groovy reader will keep
trying to read even though it has already been closed. This means any
lamda with a CSVReader was an IOException. The only way we could work
around it was to check the reader on each read.

It was not until several months after OpenCSV 3.0 went out did we find out
that by default the reader created by channels would return not ready until
after the first read. Which was why we added the verify reader.

You can use that for slow networks as well

CSVReaderBuilder builder = new CSVReaderBuilder(reader);

CSVReader reader = builder.withVerifyReader(false).build();

Sorry for the inconvenience that may have caused. If we had known
that developers were streaming over a network we would have made the
old behavior (verify reader being false) the default behavior. But at
the time we assumed it would just be common that everyone would rather
check the reader before reading and that the only time it would not be
ready was when it was closed (not because the network was lagging
behind).

If you want a best of both worlds approach where you check the reader
but want to take into account the delay is to either write your own
reader or extend the reader class you are using and override the ready
to call the super.ready() and if false wait for a passed in or
predetermined amount of milliseconds before trying again and return
that value. That will give you a chance to wait for the network to
catch up.

If you have any suggestions on alternative methods of checking the
reader to see if the file has been closed I would be happy to hear
them.

Sincerely

Scott :)

On Mon, Apr 13, 2015 at 7:17 PM, Stephen A. Goss postfuturist@users.sf.net
wrote:

I see this was already reported with issue 108, but there is actually no
case where you'd want this behavior. Normal testing doesn't reveal when a
buffer might drain due to slow IO. I only found this in integration
testing, where on a slower network, my CSVs were coming up short. It had
nothing to do with Channels either.

---

• [bugs:#109] http://sourceforge.net/p/opencsv/bugs/109 CSVReader
  mishandles BufferedReader, cutting off input*

Status: open
Group: v1.0 (example)
Created: Mon Apr 13, 2015 10:37 PM UTC by Stephen A. Goss
Last Updated: Mon Apr 13, 2015 10:37 PM UTC
Owner: nobody

The CSVReader falsely assumes that the BufferedReader is closed when it is
not ready. If the slowness of the input IO feeding the buffer is slower
than the parsing itself, the buffer might not be ready, which opencsv
considers "closed" and stops parsing erroneously.

/**     * Checks to see if the file is closed.     * @return true if the reader can no longer be read from.     */
private boolean isClosed() {
    if (!verifyReader) {
        return false;
    }
    try {
        return !br.ready(); // WRONG!!! buffer might just be waiting
    } catch (IOException e) {
        return true;
    }
}

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/opencsv/bugs/109/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

--
Scott Conway
scott.conway@gmail.com
http://www.conwayfamily.name

[Stephen A. Goss]

If you really think this is necessary it should be opt-in, not opt out. This code should do the right thing:

    /**

     * Checks to see if the file is closed.
     * @return true if the reader can no longer be read from.
     */
    private boolean isClosed() {
        if (!verifyReader) {
            return false;
        }
        try {
            br.mark(1);
            int nextByte = br.read();
            br.reset(); // resets stream position, possible because its buffered
            return nextByte == -1; // read() returns -1 at end of stream
        } catch (IOException e) {
            return true;
        }
    }

[Scott Conway]

I like where you are going with that but I believe Maciek looked at a similar solution initially. The problem is both mark and reset will throw IOException if the class implementing the Reader interface does not support that method:

http://docs.oracle.com/javase/7/docs/api/java/io/Reader.html#mark(int)

http://docs.oracle.com/javase/7/docs/api/java/io/Reader.html#reset()

And even though the reader has a markSupported() method it does not have a reset supported method.

So you could still end up with an issue like bug #108 where depending on which reader you are using the method will incorrectly tell you the file is closed when it is still open. InputStreamReader is one of those classes.

Still it just puts us back in the same position as with #108 which is why I coded the verifyReader. Once I get my workwork and familywork done (I am on mini-hiatus until May-June) I will see if I can recreate the original Groovy issue with Java 8 lambdas with the verify set to false and then test out the code as is (with ready()) and your fix and see if they work the same. I will also check to see how many other native reader classes do not support mark and reset.

Until then I would suggest just setting the verifyReader to false so you are mimicking the opencsv 2.3 functionality.

[Stephen A. Goss]

mark and reset are safe on a BufferedReader, which is what we have--the constructor of CSVReader wraps the Reader in a BufferedReader if it is not already a BufferedReader. http://docs.oracle.com/javase/7/docs/api/java/io/BufferedReader.html#mark(int)

[Tom]

I've encountered this issue too.

I'm use Google's GAE and reading a CSV file located in their GCS (their file/blob storage service).

Switching to opencsv 2.3 fixed the problem.

[Stephen A. Goss]

You can also work around the problem by disabling verifyReader using the CSVReader builder: new CSVReaderBuilder(reader).withVerifyReader(false).build()

[Tom]

Oh, OK, thanks. I've switched to 2.x now and its working so I'll stick with that until something else pulls me back to 3.x.

[Scott Conway]

Fixed - merged in trunk