On Mon, Sep 7, 2015 at 6:38 PM, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote:
> Stefan Fuhrmann <stefan.fuhrm...@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 >