[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1561387 - in /subversion/trunk/subversion: bindings/javahl/native/ include/ libsvn_ra_svn/ svnserve/ tests/libsvn_ra/

From: Branko Čibej <brane_at_wandisco.com>
Date: Sat, 25 Jan 2014 22:47:34 +0100

On 25.01.2014 21:16, rhuijben_at_apache.org wrote:
> Author: rhuijben
> Date: Sat Jan 25 20:16:57 2014
> New Revision: 1561387
>
> URL: http://svn.apache.org/r1561387
> Log:
> Following up on r1531610 and related revisions, extend the ra-svn tunnel
> creation callback to allow providing Subversion streams instead of only
> apr_file_t * handles to allow hooking in third party libraries without
> requiring external applications and/or advanced multithread synchronization
> in any application that wants to use this.

You do realize that with this change, any tunnel implementation will
always violate the svn_stream API promises?

 * The read and write handlers accept length arguments via pointer.
 * On entry to the handler, the pointed-to value should be the amount
 * of data which can be read or the amount of data to write. When the
 * handler returns, the value is reset to the amount of data actually
 * read or written. Handlers are obliged to complete a read or write
 * to the maximum extent possible; thus, a short read with no
 * associated error implies the end of the input stream, and a short
 * write should never occur without an associated error.

Note especialy this: "a short read with no associated error implies the
end of the input stream". This simply is not true for the ra_svn
protocol: the client can expect to always write the whole buffer to the
server, but reading less than the client's allocated buffer does *not*
mean that the server has closed the connection.

The JavaHL implementation of the tunnel callbacks is a good example: it
can use apr_file_write_full on the request end of the pipe, but only
plain apr_file_read on the response end, otherwise you get a deadlock
when the server has sent a response, and is waits for the client, while
the client is waits for its read buffer to fill up.

You'll note that read_handler_apr (which used by streams returned from
svn_stream_from_aprfile2) use svn_io_file_read_full2; in other words, if
you use this changed tunnel handler to integrate libssh2 into the
Windows build, you're going to get deadlocks on svn+ssh://; and so will
any other callback implementation (except the Java one, which uses
proper pipes and does not use full-read).

Our stream API is quite useless for handling ra_svn, because it does not
provide explicit end-of-stream detection; on other words, it's not a
real stream API, it assumes that there's always a file at the end of the
stream chain; which is not even close to being true in ra_svn.

And a last note: changing the pipe to a stream doesn't make it easier
for implementers, even your libssh2 example. You're just moving the
responsibility for managing the pipe somewhere else.

-1 for all the reasons stated above, please revert.

— Brane

-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com
Received on 2014-01-25 22:48:14 CET

This is an archived mail posted to the Subversion Dev mailing list.