Fix InputStreamByteChunkProvider#isAvailable

[vorobii-vitalii]

Hello!

There is issue in this function -

smbj/src/main/java/com/hierynomus/smbj/io/InputStreamByteChunkProvider.java

Line 53 in b78c872

return super.isAvailable() || (is != null && is.available() > 0);

.
According to docs - https://docs.oracle.com/javase/8/docs/api/java/io/BufferedInputStream.html. BufferedInputStream#available returns only estimate of the number of bytes that can be read.
And one of implementations of InputStream we rely on actually always returns zero before reading.
Please review my approach. I've tested it and it works as expected

[vorobii-vitalii]

@hierynomus Any news??

[laeubi]

@vorobii-vitalii As you noticed using the isAvailable for anything really is unreliable I wonder where/who it is used, maybe one better fix that place to not use isAvailable at all...

[vorobii-vitalii]

Hi, @laeubi
I agree that relying on BufferedInputStream#available is a bad idea, since it returns estimate.
However, if we want to read from some input stream in chunks and transfer chunks to Samba we still need to know whether underlying ByteChunkProvider can still provide some bytes. We shouldn't exit following loop until we are sure ByteChunkProvider won't provide any bytes.

public long write(ByteChunkProvider provider, ProgressListener progressListener) {
        int bytesWritten = 0;
        **while (provider.isAvailable()) {**
            logger.debug("Writing to {} from offset {}", this.entryName, provider.getOffset());
            SMB2WriteResponse wresp = share.write(fileId, provider);
            bytesWritten += wresp.getBytesWritten();
            if (progressListener != null)
                progressListener.onProgressChanged(wresp.getBytesWritten(), provider.getOffset());
        }
        return bytesWritten;
    }

Perhaps function name can be improved

[vorobii-vitalii]

And I am not sure we can avoid this polling loop, we have to read from file/socket in chunks, because otherwise we might run into out of memory error

[laeubi]

I agree that relying on BufferedInputStream#available is a bad idea, since it returns estimate.

It does not, per javadoc it might return an estimate it is also legal to just return 0 and this is the case you are encountering here.

So this PR actually makes isAviable to something like "wasEOF read" what already is part of the API by returning -1 ...

And I am not sure we can avoid this polling loop, we have to read from file/socket in chunks, because otherwise we might run into out of memory error

The provider need to know if there is more to write but this should not result in using available on an input stream! Instead it should work similar to how Iterator works, that is a call to hasNext() will actually perform a read of the next data and cache it somewhere or return false (in this case e.g. it has read EOF). then next() can use this cached data (what might be a small chunck) to write it / return it / whatever...

[vorobii-vitalii]

I agree that relying on BufferedInputStream#available is a bad idea, since it returns estimate.

It does not, per javadoc it might return an estimate it is also legal to just return 0 and this is the case you are encountering here.

So this PR actually makes isAviable to something like "wasEOF read" what already is part of the API by returning -1 ...

And I am not sure we can avoid this polling loop, we have to read from file/socket in chunks, because otherwise we might run into out of memory error

The provider need to know if there is more to write but this should not result in using available on an input stream! Instead it should work similar to how Iterator works, that is a call to hasNext() will actually perform a read of the next data and cache it somewhere or return false (in this case e.g. it has read EOF). then next() can use this cached data (what might be a small chunck) to write it / return it / whatever...

I agree that my solution changes semantics of this function, implemented different approach

[laeubi]

I think this is a much better approach, just some style comments.

[hierynomus]

Before I merge this change in. I'd like to see a testcase that proves the original implementation wrong.

[laeubi]

@hierynomus there is already a testcase in this PR, @vorobii-vitalii does this testcase fails without your patch?

[hierynomus]

I've seen the testcase indeed, I'm not convinced it fails without.

[vorobii-vitalii]

It was test case for new component. Added new one
With previous implementation it fails:

With new it passes:

[vorobii-vitalii]

@hierynomus @laeubi Hey guys! Is PR still missing some changes required for merge?

[laeubi]

It looks good to me but I'm not a committer on the project :-)

[laeubi]

Does it really requires a BufferedInputStream here? If not then a plain InputStream would make this clas more reusable

[vorobii-vitalii]

Hello @hierynomus Please let me know what changes are missing in this PR.