Re: svn commit: r1561688 - in /subversion/trunk/subversion: include/svn_io.h libsvn_subr/deprecated.c libsvn_subr/stream.c

[Branko Čibej <br...@wandisco.com>]

On 27.01.2014 15:16, rhuijben@apache.org wrote:
> Author: rhuijben
> Date: Mon Jan 27 14:16:08 2014
> New Revision: 1561688
>
> URL: http://svn.apache.org/r1561688
> Log:
> Following up on r1561387, which was (temporarily) reverted in r1561424 extend
> the existing stream api with an optional incomplete read function to allow
> using the stream api as was intended by r1561387.
>
> After discussion on irc we came with the idea of revving svn_stream_read()
> to two separate functions: one with full read semantics (like before) and one
> with the new possibly incomplete read support.
>
> This patch currently provides a default implementation of the incomplete read
> with a forced complete read. Brane suggests that it might be better to just
> return an error for requested incomplete reads. I commit this version under
> the assumption that it is very easy to change this later in a separate patch.

I'll explain why ...


> +void
> +svn_stream_set_read(svn_stream_t *stream,
> +                    svn_read_fn_t read_fn)
> +{
> +  svn_stream_set_read2(stream, read_fn, read_fn);
> +}

Users of the deprecated svn_stream_set_read will presumably make read_fn
conform to the full-read semantics. If we set a full-read handler as the
partial-read, some other user of the stream could cal svn_stream_read2,
expecting to get a partial read but getting a full read; which could
lead to hard-to-diagnose deadlocks.

It is IMO much better to change the implementation to this:

  svn_stream_set_read2(stream, NULL, read_fn);

so that calling svn_stream_read2 on that stream (or a stream that wraps
it, possibly in a way not visible to the consumer) will cause an
assertion, which is a lot easier to deal with than random deadlocks.


-- Brane


-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com