You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/04/28 15:45:42 UTC
svn commit: r1676535 -
/subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Author: ivan
Date: Tue Apr 28 13:45:42 2015
New Revision: 1676535
URL: http://svn.apache.org/r1676535
Log:
Follow-up to r1676526: Close install_stream in test.
* subversion/tests/libsvn_subr/io-test.c
(test_install_stream_to_longpath): Close stream before calling
svn_stream__install_stream().
Modified:
subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Modified: subversion/trunk/subversion/tests/libsvn_subr/io-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/io-test.c?rev=1676535&r1=1676534&r2=1676535&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/io-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/io-test.c Tue Apr 28 13:45:42 2015
@@ -768,6 +768,7 @@ test_install_stream_to_longpath(apr_pool
final_abspath = svn_dirent_join(deep_dir, "stream1", pool);
SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
SVN_ERR(svn_stream_puts(stream, "stream1 content"));
+ SVN_ERR(svn_stream_close(stream));
SVN_ERR(svn_stream__install_stream(stream,
final_abspath,
TRUE,
Re: svn commit: r1676535 -/subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@wandisco.com>.
On 28.04.2015 18:54, Bert Huijben wrote:
> Not on a pc, but this uses a Windows function that allows moving the
> file while it is still open. Normally when the file is reopened for
> moving by the os, virus scanners will lock the file until they are
> sure they are safe. In many cases involving our retry logic)
>
> This moves this check to *after we are done* instead of between the
> flush and the rename. So the virus scanner still does its work but
> without delaying file installs.
>
> The performance improvement is quite visible with virus scanners and
> on network shares where this avoids quite a but I client-server
> synchronization. (Even more when combined)
>
> Bert
>
> ------------------------------------------------------------------------
> From: Ivan Zhakov <ma...@visualsvn.com>
> Sent: 28-4-2015 17:48
> To: Philip Martin <ma...@wandisco.com>
> Cc: dev@subversion.apache.org <ma...@subversion.apache.org>;
> brane@apache.org <ma...@apache.org>
> Subject: Re: svn commit: r1676535
> -/subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>
> On 28 April 2015 at 18:42, Philip Martin <ph...@wandisco.com>
> wrote:
> > Ivan Zhakov <iv...@visualsvn.com> writes:
> >
> >>> What does "can be installed using Windows checkouts much slower than
> >>> Unix" mean?
> >>>
> >> I have no idea. This function was added in r1559758 [1] as preparation
> >> to fix issue #4450 "Windows checkouts much slower then Unix" [2]. May
> >> issue summary leaked to function docstring for some reason.
> >
> > I suspect it should be "can be installed using platform specific
> > optimizations". Perhaps with something additional about closing the
> > stream before installing.
> >
> > Is closing before installing just allowed or is it required? Before
> > your commit I wrote a patch that closed the stream *after* calling
> > svn_stream__install_stream and that was sufficient for the test to pass.
> >
> As far I understand from learning code close is *required* and should
> be performed *before* calling svn_stream__install_stream(). Calling
> svn_stream_close() after svn_stream__install_stream() works because OS
> just flushes buffers to new location, but it's platform specific
> behavior and data may be lost if power off between move and flush for
> some reason.
I think the reason for "requiring" the close before install is that we
don't have an svn_stream_flush function. In the case of install streams,
the close handler is hacked to flush the file on Windows, and the stream
knows the underlying file name, so it the seemingly illogical sequence
of "close then use" happens to work. It's just not intuitive.
-- Brane
RE: svn commit: r1676535
-/subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
Not on a pc, but this uses a Windows function that allows moving the file while it is still open. Normally when the file is reopened for moving by the os, virus scanners will lock the file until they are sure they are safe. In many cases involving our retry logic)
This moves this check to *after we are done* instead of between the flush and the rename. So the virus scanner still does its work but without delaying file installs.
The performance improvement is quite visible with virus scanners and on network shares where this avoids quite a but I client-server synchronization. (Even more when combined)
Bert
-----Original Message-----
From: "Ivan Zhakov" <iv...@visualsvn.com>
Sent: 28-4-2015 17:48
To: "Philip Martin" <ph...@wandisco.com>
Cc: "dev@subversion.apache.org" <de...@subversion.apache.org>; "brane@apache.org" <br...@apache.org>
Subject: Re: svn commit: r1676535 -/subversion/trunk/subversion/tests/libsvn_subr/io-test.c
On 28 April 2015 at 18:42, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>>> What does "can be installed using Windows checkouts much slower than
>>> Unix" mean?
>>>
>> I have no idea. This function was added in r1559758 [1] as preparation
>> to fix issue #4450 "Windows checkouts much slower then Unix" [2]. May
>> issue summary leaked to function docstring for some reason.
>
> I suspect it should be "can be installed using platform specific
> optimizations". Perhaps with something additional about closing the
> stream before installing.
>
> Is closing before installing just allowed or is it required? Before
> your commit I wrote a patch that closed the stream *after* calling
> svn_stream__install_stream and that was sufficient for the test to pass.
>
As far I understand from learning code close is *required* and should
be performed *before* calling svn_stream__install_stream(). Calling
svn_stream_close() after svn_stream__install_stream() works because OS
just flushes buffers to new location, but it's platform specific
behavior and data may be lost if power off between move and flush for
some reason.
--
Ivan Zhakov
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 April 2015 at 18:42, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>>> What does "can be installed using Windows checkouts much slower than
>>> Unix" mean?
>>>
>> I have no idea. This function was added in r1559758 [1] as preparation
>> to fix issue #4450 "Windows checkouts much slower then Unix" [2]. May
>> issue summary leaked to function docstring for some reason.
>
> I suspect it should be "can be installed using platform specific
> optimizations". Perhaps with something additional about closing the
> stream before installing.
>
> Is closing before installing just allowed or is it required? Before
> your commit I wrote a patch that closed the stream *after* calling
> svn_stream__install_stream and that was sufficient for the test to pass.
>
As far I understand from learning code close is *required* and should
be performed *before* calling svn_stream__install_stream(). Calling
svn_stream_close() after svn_stream__install_stream() works because OS
just flushes buffers to new location, but it's platform specific
behavior and data may be lost if power off between move and flush for
some reason.
--
Ivan Zhakov
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
>> What does "can be installed using Windows checkouts much slower than
>> Unix" mean?
>>
> I have no idea. This function was added in r1559758 [1] as preparation
> to fix issue #4450 "Windows checkouts much slower then Unix" [2]. May
> issue summary leaked to function docstring for some reason.
I suspect it should be "can be installed using platform specific
optimizations". Perhaps with something additional about closing the
stream before installing.
Is closing before installing just allowed or is it required? Before
your commit I wrote a patch that closed the stream *after* calling
svn_stream__install_stream and that was sufficient for the test to pass.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 April 2015 at 18:32, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>>>> SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
>>>> SVN_ERR(svn_stream_puts(stream, "stream1 content"));
>>>> + SVN_ERR(svn_stream_close(stream));
>>>> SVN_ERR(svn_stream__install_stream(stream,
>>>> final_abspath,
>>>> TRUE,
>>>
>>>
>>> Um, surely that can't be right? Using the stream after it's been closed?
>>>
>> I'm also find this behavior a little bit confusing, but this is how it
>> works now: install_stream should be closed before installing. Windows
>> implementation perform flush on close, while unix implementation
>> closes the file handle. The current callers (run_file_install in
>> workqueue.c:587) also closes install_stream before installing it.
>
> While svn_stream__install_stream doesn't explicitly document that a
> closed stream can be passed it is part of the documentation for
> svn_stream__install_get_info.
>
> The documentation for svn_stream__create_for_install appears to have got
> mangled:
>
> Creates as *INSTALL_STREAM a stream that once completed can be installed
> using Windows checkouts much slower than Unix.
>
> What does "can be installed using Windows checkouts much slower than
> Unix" mean?
>
I have no idea. This function was added in r1559758 [1] as preparation
to fix issue #4450 "Windows checkouts much slower then Unix" [2]. May
issue summary leaked to function docstring for some reason.
[1] https://svn.apache.org/r1559758
[2] http://subversion.tigris.org/issues/show_bug.cgi?id=4450
--
Ivan Zhakov
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
>>> SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
>>> SVN_ERR(svn_stream_puts(stream, "stream1 content"));
>>> + SVN_ERR(svn_stream_close(stream));
>>> SVN_ERR(svn_stream__install_stream(stream,
>>> final_abspath,
>>> TRUE,
>>
>>
>> Um, surely that can't be right? Using the stream after it's been closed?
>>
> I'm also find this behavior a little bit confusing, but this is how it
> works now: install_stream should be closed before installing. Windows
> implementation perform flush on close, while unix implementation
> closes the file handle. The current callers (run_file_install in
> workqueue.c:587) also closes install_stream before installing it.
While svn_stream__install_stream doesn't explicitly document that a
closed stream can be passed it is part of the documentation for
svn_stream__install_get_info.
The documentation for svn_stream__create_for_install appears to have got
mangled:
Creates as *INSTALL_STREAM a stream that once completed can be installed
using Windows checkouts much slower than Unix.
What does "can be installed using Windows checkouts much slower than
Unix" mean?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@wandisco.com>.
On 28.04.2015 17:03, Ivan Zhakov wrote:
> On 28 April 2015 at 17:56, Branko Čibej <br...@wandisco.com> wrote:
>> On 28.04.2015 15:45, ivan@apache.org wrote:
>>> Author: ivan
>>> Date: Tue Apr 28 13:45:42 2015
>>> New Revision: 1676535
>>>
>>> URL: http://svn.apache.org/r1676535
>>> Log:
>>> Follow-up to r1676526: Close install_stream in test.
>>>
>>> * subversion/tests/libsvn_subr/io-test.c
>>> (test_install_stream_to_longpath): Close stream before calling
>>> svn_stream__install_stream().
>>>
>>> Modified:
>>> subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>>>
>>> Modified: subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/io-test.c?rev=1676535&r1=1676534&r2=1676535&view=diff
>>> ==============================================================================
>>> --- subversion/trunk/subversion/tests/libsvn_subr/io-test.c (original)
>>> +++ subversion/trunk/subversion/tests/libsvn_subr/io-test.c Tue Apr 28 13:45:42 2015
>>> @@ -768,6 +768,7 @@ test_install_stream_to_longpath(apr_pool
>>> final_abspath = svn_dirent_join(deep_dir, "stream1", pool);
>>> SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
>>> SVN_ERR(svn_stream_puts(stream, "stream1 content"));
>>> + SVN_ERR(svn_stream_close(stream));
>>> SVN_ERR(svn_stream__install_stream(stream,
>>> final_abspath,
>>> TRUE,
>>
>> Um, surely that can't be right? Using the stream after it's been closed?
>>
> I'm also find this behavior a little bit confusing, but this is how it
> works now: install_stream should be closed before installing. Windows
> implementation perform flush on close, while unix implementation
> closes the file handle. The current callers (run_file_install in
> workqueue.c:587) also closes install_stream before installing it.
Ah, I see it now ... "install streams" are special, they store the file
name in the stream baton.
-- Brane
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 April 2015 at 17:56, Branko Čibej <br...@wandisco.com> wrote:
> On 28.04.2015 15:45, ivan@apache.org wrote:
>> Author: ivan
>> Date: Tue Apr 28 13:45:42 2015
>> New Revision: 1676535
>>
>> URL: http://svn.apache.org/r1676535
>> Log:
>> Follow-up to r1676526: Close install_stream in test.
>>
>> * subversion/tests/libsvn_subr/io-test.c
>> (test_install_stream_to_longpath): Close stream before calling
>> svn_stream__install_stream().
>>
>> Modified:
>> subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>>
>> Modified: subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/io-test.c?rev=1676535&r1=1676534&r2=1676535&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/tests/libsvn_subr/io-test.c (original)
>> +++ subversion/trunk/subversion/tests/libsvn_subr/io-test.c Tue Apr 28 13:45:42 2015
>> @@ -768,6 +768,7 @@ test_install_stream_to_longpath(apr_pool
>> final_abspath = svn_dirent_join(deep_dir, "stream1", pool);
>> SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
>> SVN_ERR(svn_stream_puts(stream, "stream1 content"));
>> + SVN_ERR(svn_stream_close(stream));
>> SVN_ERR(svn_stream__install_stream(stream,
>> final_abspath,
>> TRUE,
>
>
> Um, surely that can't be right? Using the stream after it's been closed?
>
I'm also find this behavior a little bit confusing, but this is how it
works now: install_stream should be closed before installing. Windows
implementation perform flush on close, while unix implementation
closes the file handle. The current callers (run_file_install in
workqueue.c:587) also closes install_stream before installing it.
--
Ivan Zhakov
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@wandisco.com>.
On 28.04.2015 15:45, ivan@apache.org wrote:
> Author: ivan
> Date: Tue Apr 28 13:45:42 2015
> New Revision: 1676535
>
> URL: http://svn.apache.org/r1676535
> Log:
> Follow-up to r1676526: Close install_stream in test.
>
> * subversion/tests/libsvn_subr/io-test.c
> (test_install_stream_to_longpath): Close stream before calling
> svn_stream__install_stream().
>
> Modified:
> subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>
> Modified: subversion/trunk/subversion/tests/libsvn_subr/io-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/io-test.c?rev=1676535&r1=1676534&r2=1676535&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_subr/io-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_subr/io-test.c Tue Apr 28 13:45:42 2015
> @@ -768,6 +768,7 @@ test_install_stream_to_longpath(apr_pool
> final_abspath = svn_dirent_join(deep_dir, "stream1", pool);
> SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
> SVN_ERR(svn_stream_puts(stream, "stream1 content"));
> + SVN_ERR(svn_stream_close(stream));
> SVN_ERR(svn_stream__install_stream(stream,
> final_abspath,
> TRUE,
Um, surely that can't be right? Using the stream after it's been closed?
-- Brane
Re: svn commit: r1676535 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
Posted by Branko Čibej <br...@wandisco.com>.
On 28.04.2015 15:45, ivan@apache.org wrote:
> Author: ivan
> Date: Tue Apr 28 13:45:42 2015
> New Revision: 1676535
>
> URL: http://svn.apache.org/r1676535
> Log:
> Follow-up to r1676526: Close install_stream in test.
>
> * subversion/tests/libsvn_subr/io-test.c
> (test_install_stream_to_longpath): Close stream before calling
> svn_stream__install_stream().
>
> Modified:
> subversion/trunk/subversion/tests/libsvn_subr/io-test.c
>
> Modified: subversion/trunk/subversion/tests/libsvn_subr/io-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/io-test.c?rev=1676535&r1=1676534&r2=1676535&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_subr/io-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_subr/io-test.c Tue Apr 28 13:45:42 2015
> @@ -768,6 +768,7 @@ test_install_stream_to_longpath(apr_pool
> final_abspath = svn_dirent_join(deep_dir, "stream1", pool);
> SVN_ERR(svn_stream__create_for_install(&stream, deep_dir, pool, pool));
> SVN_ERR(svn_stream_puts(stream, "stream1 content"));
> + SVN_ERR(svn_stream_close(stream));
> SVN_ERR(svn_stream__install_stream(stream,
> final_abspath,
> TRUE,
Um, surely that can't be right? Using the stream after it's been closed?
-- Brane