Обсуждение: jdbc1.AbstractJdbc1Statement.setBinaryStream bug and patch
Hello,
org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream()
in postgresql 7.4.1 wrongly assumes, that
java.io.InputStream.read(byte[] b,int offset,int len )
will always read len bytes. InputStream only guarantees to
return at least 1 byte per call. The attached patch solves the bug.
Btw. setBinaryStream() should really throw an SQLException, if
in can not read as many bytes as expected from the InputStream.
Otherwise the application might silently loss data.
Regards
Martin
--
Martin Holz <holz@fiz-chemie.de>
Softwareentwicklung / Vernetztes Studium - Chemie
FIZ CHEMIE Berlin
Franklin Str. 11
D-10587 Berlin
On Wed, 7 Jan 2004, Martin Holz wrote: > Hello, > > org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream() > in postgresql 7.4.1 wrongly assumes, that > java.io.InputStream.read(byte[] b,int offset,int len ) > will always read len bytes. InputStream only guarantees to > return at least 1 byte per call. The attached patch solves the bug. Yes, it can even return 0 bytes if it feels like it. > Btw. setBinaryStream() should really throw an SQLException, if > in can not read as many bytes as expected from the InputStream. > Otherwise the application might silently loss data. The odd thing about the javadocs is that they say "The data will be read from the stream as needed until end-of-file is reached." The length parameter's comment is "the number of bytes in the stream." This is strange because it seems to imply that the length argument is just for informational purposes and should not be used to limit the amount of data actually read from the stream. Kris Jurka
On Friday 09 January 2004 09:46, Kris Jurka wrote: > On Wed, 7 Jan 2004, Martin Holz wrote: > > Hello, > > > > org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream() > > in postgresql 7.4.1 wrongly assumes, that > > java.io.InputStream.read(byte[] b,int offset,int len ) > > will always read len bytes. InputStream only guarantees to > > return at least 1 byte per call. The attached patch solves the > > bug. > > Yes, it can even return 0 bytes if it feels like it. Only if len is 0. From javadoc: * If len is zero, then no bytes are read and 0 is returned; * otherwise, there is an attempt to read at least one byte. If no * byte is available because the stream is at end of file, the value * -1 is returned; otherwise, at least one byte is read and stored * into b. > The odd thing about the javadocs is that they say "The data will be > read from the stream as needed until end-of-file is reached." The > length parameter's comment is "the number of bytes in the stream." Don't know, which version of Javadoc you use. http://java.sun.com/j2se/1.4.2/docs/api/java/io/InputStream.html#read(byte[],%20int,%20int) seems pretty clear to me. > This is strange because it seems to imply that the length argument > is just for informational purposes and should not be used to limit > the amount of data actually read from the stream. As I read the documentation, unless len == 0, at least 1, but not more then len bytes are read. If no input is available the method will block. Otherwise the method will return len bytes if possible,but less if it would block otherwise. Martin -- Martin Holz <holz@fiz-chemie.de> Softwareentwicklung / Vernetztes Studium - Chemie FIZ CHEMIE Berlin Franklin Str. 11 D-10587 Berlin
On Friday 09 January 2004 09:46, Kris Jurka wrote: > On Wed, 7 Jan 2004, Martin Holz wrote: > > Hello, > > > > org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream() > > in postgresql 7.4.1 wrongly assumes, that > > java.io.InputStream.read(byte[] b,int offset,int len ) > > will always read len bytes. InputStream only guarantees to > > return at least 1 byte per call. The attached patch solves the > > bug. > > Yes, it can even return 0 bytes if it feels like it. > > > Btw. setBinaryStream() should really throw an SQLException, if > > in can not read as many bytes as expected from the InputStream. > > Otherwise the application might silently loss data. > > The odd thing about the javadocs is that they say "The data will be > read from the stream as needed until end-of-file is reached." The > length parameter's comment is "the number of bytes in the stream." You seem to use a other javadoc than me. http://java.sun.com/j2se/1.4.2/docs/api/java/io/InputStream.html#read(byte[],%20int,%20int) is pretty clear. > This is strange because it seems to imply that the length argument > is just for informational purposes and should not be used to limit > the amount of data actually read from the stream. At least 1 byte will be read unless len == 0. No more than len bytes will be read. Within this limit read is free to return as many bytes as it considers efficient. Martin -- Martin Holz <holz@fiz-chemie.de> Softwareentwicklung / Vernetztes Studium - Chemie FIZ CHEMIE Berlin Franklin Str. 11 D-10587 Berlin
On Fri, 9 Jan 2004, Martin Holz wrote: > On Friday 09 January 2004 09:46, Kris Jurka wrote: > > > The odd thing about the javadocs is that they say "The data will be > > read from the stream as needed until end-of-file is reached." The > > length parameter's comment is "the number of bytes in the stream." > > Don't know, which version of Javadoc you use. > > http://java.sun.com/j2se/1.4.2/docs/api/java/io/InputStream.html#read(byte[],%20int,%20int) > seems pretty clear to me. > > > This is strange because it seems to imply that the length argument > > is just for informational purposes and should not be used to limit > > the amount of data actually read from the stream. > I was referring to the documentation for setBinaryStream. You mentioned that it should throw an Exception if it couldn't read "length" bytes from the stream. I was commenting that our implementation might be wrong by checking the length at all. Kris Jurka
On Friday 09 January 2004 17:18, Kris Jurka wrote:
> I was referring to the documentation for setBinaryStream. You
> mentioned that it should throw an Exception if it couldn't read
> "length" bytes from the stream. I was commenting that our
> implementation might be wrong by checking the length at all.
Sorry for not being precise. There are two different issues.
1) The way the InputStream is read is clearly wrong.
This should be fixed.
2) What to do, when the size of the input stream does
not match the lenght argument of Statement.setBinaryStream()
There are two options
a) Throw a exception
b) Silently send all bytes, that are available.
I don't really understand what the length argument is good for
and think, that this is a design flaw made by Sun. I would prefer
a), but I am not sure here.
Martin
--
Martin Holz <holz@fiz-chemie.de>
Softwareentwicklung / Vernetztes Studium - Chemie
FIZ CHEMIE Berlin
Franklin Str. 11
D-10587 Berlin
On Wed, 7 Jan 2004, Martin Holz wrote: > Hello, > > org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream() > in postgresql 7.4.1 wrongly assumes, that > java.io.InputStream.read(byte[] b,int offset,int len ) > will always read len bytes. InputStream only guarantees to > return at least 1 byte per call. The attached patch solves the bug. > I have applied a version of this patch to the cvs version on gborg. The setAsciiStream and setUnicodeStream methods also had this problem. Your patch was not quite right because it didn't correctly handle the situation where the Stream was longer than the given length. Kris Jurka
Kris Jurka <books@ejurka.com> writes: > On Wed, 7 Jan 2004, Martin Holz wrote: > > > Hello, > > > > org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream() > > in postgresql 7.4.1 wrongly assumes, that > > java.io.InputStream.read(byte[] b,int offset,int len ) > > will always read len bytes. InputStream only guarantees to > > return at least 1 byte per call. The attached patch solves the bug. > > > > I have applied a version of this patch to the cvs version on gborg. The > setAsciiStream and setUnicodeStream methods also had this problem. Thank you. Why does it not show up at http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/interfaces/jdbc/org/postgresql/jdbc1/AbstractJdbc1Statement.java ? > Your > patch was not quite right because it didn't correctly handle the situation > where the Stream was longer than the given length. > You are right.
On 16 Jan 2004, Martin Holz wrote: > Kris Jurka <books@ejurka.com> writes: > > > On Wed, 7 Jan 2004, Martin Holz wrote: > > > > > Hello, > > > > > > org.postgresql.jdbc1.AbstractJdbc1Statement.setBinaryStream() > > > in postgresql 7.4.1 wrongly assumes, that > > > java.io.InputStream.read(byte[] b,int offset,int len ) > > > will always read len bytes. InputStream only guarantees to > > > return at least 1 byte per call. The attached patch solves the bug. > > > > > > > I have applied a version of this patch to the cvs version on gborg. The > > setAsciiStream and setUnicodeStream methods also had this problem. > > Thank you. Why does it not show up at > http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/interfaces/jdbc/org/postgresql/jdbc1/AbstractJdbc1Statement.java ? > All new development of the JDBC driver has moved to gborg.postgresql.org. With a homepage of http://gborg.postgresql.org/project/pgjdbc/projdisplay.php and the webcvs url you are looking for is: http://gborg.postgresql.org/project/pgjdbc/cvs/cvs.php/pgjdbc/org/postgresql/jdbc1/AbstractJdbc1Statement.java This patch should probably be applied to the stable version that will remain in the 7.4 branch in the main cvs tree, but I don't have the power to do anything about that. Kris Jurka