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
> 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.
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
Received on 2014-01-25 22:48:14 CET