On Mon, Sep 7, 2015 at 6:38 PM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
wrote:
> Stefan Fuhrmann <stefan.fuhrmann_at_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
>
Received on 2015-09-08 12:49:35 CEST