subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1700799 - in /subversion/trunk/subversion: include/svn_io.h libsvn_subr/stream.c svnadmin/svnadmin.c svnfsfs/load-index-cmd.c tests/libsvn_subr/stream-test.c
Date Tue, 08 Sep 2015 10:49:24 GMT
On Mon, Sep 7, 2015 at 6:38 PM, Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
wrote:

> Stefan Fuhrmann <stefan.fuhrmann@wandisco.com> writes:
>
> > That is indeed a good idea, so I tried it in various ways:
> >
> > Index: subversion/libsvn_subr/stream.c
> > ===================================================================
> > --- subversion/libsvn_subr/stream.c    (revision 1701330)
> > +++ subversion/libsvn_subr/stream.c    (working copy)
> > @@ -1826,10 +1826,20 @@ svn_stream_for_stdin(svn_stream_t **in,
> apr_pool_t
> >    apr_status_t apr_err;
> >
> >    apr_err = apr_file_open_stdin(&stdin_file, pool);
> > +
> > +/* Alternatively to the above: tell APR to create a buffered file
> > +  apr_err = apr_file_open_flags_stdin(&stdin_file, APR_BUFFERED, pool);
> > +*/
> >    if (apr_err)
> >      return svn_error_wrap_apr(apr_err, "Can't open stdin");
>
> The original commit begins using svn_stream_wrap_buffered_read() during
> svnadmin load-revprops and svnfsfs load-index.  This patch, however, does
> something entirely different and adds buffering to *every* stdin.
>

Sorry for the confusion. I did not intend to actually change the
implementation of svn_stream_for_stdin this way but simply
tried to demonstrate a problem with APR buffer reads for
"streamy" file handles.

I am not sure why would we want to do this, but there certainly is a reason
> against a change like this — buffering can't be used when something
> interactive
> happens using stdin, e.g., with svnserve.
>

It is certainly useful to make buffering available as an option
(like you suggested below) instead of wrapping the stream
locally or wrapping it unconditionally. Currently, I'm not 100%
sure whether always wrapping stdin would be safe but I think
it is at least for correct usage:

A read to the wrapper will translate to at most 1 read at the
APR layer, so it will never request more data from stdin than
stdin can deliver at the moment. Exception: Single byte reads
may block scenarios just as and because they do at the APR
level.

So, whatever logic a stream API user applies to determine
that some interaction is required before new data can arrive
in stdin, that logic applies 1:1 with and without the wrapper.

So, why can't we enable buffering for these subcommands (perhaps including
> svnadmin load), leave everything else as is and avoid the necessity of
> having
> and maintaining the custom buffered stream adapter?
>

The underlying problem is still present: If stdin can't deliver
data fast enough (e.g. some hick-up / long latency on the
producer side of a dump | load), a buffered APR file will
error out while a non-buffered one will simply wait & retry.

However, I have yet to try and provoke the error specifically
for the dump | load scenario.

We could introduce svn_stream_for_stdin2(..., svn_boolean_t buffered, ...),
> and then the required change for svnadmin load-revprops would look as below
> (changes for other subcommands would look almost exactly the same):
>

Regardless of the buffering method used, +1 on adding
a rev'ed API.

-- Stefan^2.


>
> [[[
> Index: subversion/libsvn_subr/stream.c
> ===================================================================
> --- subversion/libsvn_subr/stream.c (revision 1701648)
> +++ subversion/libsvn_subr/stream.c (working copy)
> @@ -1820,12 +1820,20 @@ svn_stream_wrap_buffered_read(svn_stream_t *inner,
>
>
>  svn_error_t *
> -svn_stream_for_stdin(svn_stream_t **in, apr_pool_t *pool)
> +svn_stream_for_stdin2(svn_stream_t **in,
> +                      svn_boolean_t buffered,
> +                      apr_pool_t *pool)
>  {
>    apr_file_t *stdin_file;
>    apr_status_t apr_err;
> +  apr_int32_t flags;
>
> -  apr_err = apr_file_open_stdin(&stdin_file, pool);
> +  if (buffered)
> +    flags = APR_BUFFERED;
> +  else
> +    flags = 0;
> +
> +  apr_err = apr_file_open_flags_stdin(&stdin_file, flags, pool);
>    if (apr_err)
>      return svn_error_wrap_apr(apr_err, "Can't open stdin");
>
> Index: subversion/svnadmin/svnadmin.c
> ===================================================================
> --- subversion/svnadmin/svnadmin.c  (revision 1701648)
> +++ subversion/svnadmin/svnadmin.c  (working copy)
> @@ -1547,8 +1547,7 @@ subcommand_load_revprops(apr_getopt_t *os, void *b
>    SVN_ERR(open_repos(&repos, opt_state->repository_path, pool));
>
>    /* Read the stream from STDIN.  Users can redirect a file. */
> -  SVN_ERR(svn_stream_for_stdin(&stdin_stream, pool));
> -  stdin_stream = svn_stream_wrap_buffered_read(stdin_stream, pool);
> +  SVN_ERR(svn_stream_for_stdin2(&stdin_stream, TRUE, pool));
>
>    /* Progress feedback goes to STDOUT, unless they asked to suppress it.
> */
>    if (! opt_state->quiet)
>
> ]]]
>
>
> Regards,
> Evgeny Kotkov
>

Mime
View raw message