You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2014/03/18 19:32:01 UTC
client side workaround for svnserve iprops bug
philip@apache.org writes:
> Author: philip
> Date: Tue Mar 18 12:57:22 2014
> New Revision: 1578853
>
> URL: http://svn.apache.org/r1578853
> Log:
> Make svnserve recognise when the client does not want inherited
> properties. This fixes a performance problem with 1.8 servers
> sending too much data.
An implementation bug causes svnserve to mishandle the nominally
optional want-iprops flag for get-dir and get-file with the result that
the server keeps sending properties even when the client doesn't want
them, this can be a serious performance regression. (The want-iprops
flags was documented/released with 1.8 but isn't used by the released
client which uses get-iprops instead.) The problem is fixed on trunk
and nominated for 1.8 with 1578853.
We could add a client-side workaround for buggy servers, see the patch
below. We could do this for trunk, 1.8, even 1.7. Is it a good thing
to do?
Index: subversion/libsvn_ra_svn/client.c
===================================================================
--- subversion/libsvn_ra_svn/client.c (revision 1578900)
+++ subversion/libsvn_ra_svn/client.c (working copy)
@@ -1371,7 +1371,10 @@ static svn_error_t *ra_svn_get_dir(svn_ra_session_
if (dirent_fields & SVN_DIRENT_LAST_AUTHOR)
SVN_ERR(svn_ra_svn__write_word(conn, pool, SVN_RA_SVN_DIRENT_LAST_AUTHOR));
- SVN_ERR(svn_ra_svn__write_tuple(conn, pool, "!))"));
+ /* Always send the, nominally optional, want-iprops as "false" to
+ workaround a bug in svnserve 1.8.0-1.8.9 that causes the server
+ to see "true" if it omitted. */
+ SVN_ERR(svn_ra_svn__write_tuple(conn, pool, "!)b)", FALSE));
SVN_ERR(handle_auth_request(sess_baton, pool));
SVN_ERR(svn_ra_svn__read_cmd_response(conn, pool, "rll", &rev, &proplist,
Index: subversion/libsvn_ra_svn/marshal.c
===================================================================
--- subversion/libsvn_ra_svn/marshal.c (revision 1578900)
+++ subversion/libsvn_ra_svn/marshal.c (working copy)
@@ -2092,7 +2092,10 @@ svn_ra_svn__write_cmd_get_file(svn_ra_svn_conn_t *
SVN_ERR(write_tuple_end_list(conn, pool));
SVN_ERR(write_tuple_boolean(conn, pool, props));
SVN_ERR(write_tuple_boolean(conn, pool, stream));
- SVN_ERR(writebuf_write_short_string(conn, pool, ") ) ", 4));
+ /* Always send the, nominally optional, want-iprops as "false" to
+ workaround a bug in svnserve 1.8.0-1.8.9 that causes the server
+ to see "true" if it omitted. */
+ SVN_ERR(writebuf_write_short_string(conn, pool, " false ) ) ", 11));
return SVN_NO_ERROR;
}
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client side workaround for svnserve iprops bug
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Apr 08, 2014 at 21:28:17 +0000:
> Philip Martin wrote on Tue, Apr 08, 2014 at 20:12:26 +0100:
> > Daniel Shahaf <d....@daniel.shahaf.name> writes:
> > > We may choose to support the form that the 1.8.0-1.8.9 server code
> > > accepts (that is, "?B"), or we may choose to declare that a bug in
> > > 1.8.0-1.8.9.
> >
> > Does anyone have any opinions about which one we should choose?
> >
>
> I propose that we use the "?B" variant but require the boolean to be
> false whenever provided. Passing "true" would be an error.
Of course, 1.8.0-1.8.9 would not flag an error if a client does send
get-iprops=true. We will simply declare that failure to flag an error
a bug in those versions of svnserve.
Daniel
Re: client side workaround for svnserve iprops bug
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Tue, Apr 08, 2014 at 20:12:26 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Philip Martin wrote on Wed, Mar 19, 2014 at 13:17:08 +0000:
> >> We have:
> >>
> >> - the documented protocol
> >>
> >> (? want-iprops:boolean )
> >>
> >> - the released server implementation of the protocol
> >>
> >> ? want-iprops:boolean
> >>
> >> - the released behaviour
> >>
> >> properties always sent
> >>
> >> - the trunk behaviour
> >>
> >> properties only sent when want-iprops is true
> >>
> >> A third party client could already be using want-iprops with a 1.8
> >> server to get inherited props, even with the broken behaviour.
> >
> > Not our problem. We have no obligation to support third parties who
> > reimplement our protocol. We also have no obligation to make the server
> > accept constructions that no released client ever generated.
>
> So the ra_svn protocol is a private API. Is this a position we have
> taken in the past? I'm not opposed to it but I wasn't aware that it was
> our position.
>
I believe so but don't have a ready reference for it. Perhaps someone
else can weight in on this.
> > If I understand correctly, you're saying that no *released* client[1]
> > ever sent a want-iprops boolean on get-file and get-dir;
>
> Correct.
>
> > so we have zero
> > obligation to support a want-iprops parameter there, period. (To clarify,
> > we have no obligation to support either "?B" or "(?B)" or "?(?B)".)
> >
> > We may choose to support the form that the 1.8.0-1.8.9 server code
> > accepts (that is, "?B"), or we may choose to declare that a bug in
> > 1.8.0-1.8.9.
>
> Does anyone have any opinions about which one we should choose?
>
I propose that we use the "?B" variant but require the boolean to be
false whenever provided. Passing "true" would be an error.
That's basically a win-win situation: we gain the ability to apply the
patch without the burden of having to support or document a
"get-iprops=true" codepath that no released client uses.
This solution presumes that we are free to retroactively change
libsvn_ra_svn/protocol, i.e., that it is a private API (as you say above).
Daniel
Re: client side workaround for svnserve iprops bug
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Philip Martin wrote on Wed, Mar 19, 2014 at 13:17:08 +0000:
>> We have:
>>
>> - the documented protocol
>>
>> (? want-iprops:boolean )
>>
>> - the released server implementation of the protocol
>>
>> ? want-iprops:boolean
>>
>> - the released behaviour
>>
>> properties always sent
>>
>> - the trunk behaviour
>>
>> properties only sent when want-iprops is true
>>
>> A third party client could already be using want-iprops with a 1.8
>> server to get inherited props, even with the broken behaviour.
>
> Not our problem. We have no obligation to support third parties who
> reimplement our protocol. We also have no obligation to make the server
> accept constructions that no released client ever generated.
So the ra_svn protocol is a private API. Is this a position we have
taken in the past? I'm not opposed to it but I wasn't aware that it was
our position.
> If I understand correctly, you're saying that no *released* client[1]
> ever sent a want-iprops boolean on get-file and get-dir;
Correct.
> so we have zero
> obligation to support a want-iprops parameter there, period. (To clarify,
> we have no obligation to support either "?B" or "(?B)" or "?(?B)".)
>
> We may choose to support the form that the 1.8.0-1.8.9 server code
> accepts (that is, "?B"), or we may choose to declare that a bug in
> 1.8.0-1.8.9.
Does anyone have any opinions about which one we should choose?
> In the former case we may apply the patch.
If we decide to retain "?B", to allow further optional arguments in
future, then applying the patch has two benefits, it improves clients
using the buggy servers and it add the placeholder that will be needed
when an optional argument is added.
> In the latter case, we shouldn't apply the patch, and moreover we will
> have to "seal" get-file and get-dir --- that is, document that no
> further optional arguments may be appended to them. (That's not a
> problem --- it just means that if we want to add an argument at the end,
> we create a new protocol command "get-file2" and "get-dir2", or
> condition the new argument on a new server capability. That's to ensure
> we never send the extra arguments to a 1.8.0-1.8.9 server (which will
> misinterpret them).)
>
> Also in the latter case, we can either remove the "?B" from the server's
> code, or keep it --- released clients and third-parties following the
> documented protocol should behave the same with or without it, since
> they never send a bare "B" there.
>
> In either case, we update libsvn_ra_svn/protocol to document the
> divergence, and may choose to issue an API errata (in spite of the
> protocol spec nominally being a private API).
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client side workaround for svnserve iprops bug
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, Mar 19, 2014 at 13:17:08 +0000:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > I would vote "+1 to backport" right here and now, but I'm a bit confused
> > by the patch: it seems the client and server send and expect a bare
> > boolean, whereas libsvn_ra_svn/protocol calls for a boolean wrapped in a
> > tuple.
> >
> > (i.e., '( foo bar true ) ' vs '( foo bar ( true ) ) '.
>
> It's ugly. I believe the want-iprops flag was added to the get-dir and
> get-file commands before the get-iprops command was added. Our client
> now uses get-iprops excusively and never sends want-iprops with get-dir
> or get-file. I suspect want-iprops should not have been released.
>
> We have:
>
> - the documented protocol
>
> (? want-iprops:boolean )
>
> - the released server implementation of the protocol
>
> ? want-iprops:boolean
>
> - the released behaviour
>
> properties always sent
>
> - the trunk behaviour
>
> properties only sent when want-iprops is true
>
> A third party client could already be using want-iprops with a 1.8
> server to get inherited props, even with the broken behaviour.
Not our problem. We have no obligation to support third parties who
reimplement our protocol. We also have no obligation to make the server
accept constructions that no released client ever generated.
If I understand correctly, you're saying that no *released* client[1]
ever sent a want-iprops boolean on get-file and get-dir; so we have zero
obligation to support a want-iprops parameter there, period. (To clarify,
we have no obligation to support either "?B" or "(?B)" or "?(?B)".)
We may choose to support the form that the 1.8.0-1.8.9 server code
accepts (that is, "?B"), or we may choose to declare that a bug in
1.8.0-1.8.9.
In the former case we may apply the patch.
In the latter case, we shouldn't apply the patch, and moreover we will
have to "seal" get-file and get-dir --- that is, document that no
further optional arguments may be appended to them. (That's not a
problem --- it just means that if we want to add an argument at the end,
we create a new protocol command "get-file2" and "get-dir2", or
condition the new argument on a new server capability. That's to ensure
we never send the extra arguments to a 1.8.0-1.8.9 server (which will
misinterpret them).)
Also in the latter case, we can either remove the "?B" from the server's
code, or keep it --- released clients and third-parties following the
documented protocol should behave the same with or without it, since
they never send a bare "B" there.
In either case, we update libsvn_ra_svn/protocol to document the
divergence, and may choose to issue an API errata (in spite of the
protocol spec nominally being a private API).
> [...]
> but that would mean accepting the "? foo" form rather than "(? foo)" and
> I seem to recall that the later is preferred as it's easier to extend.
Yup. See change-rev-prop and change-rev-prop2 for an example. (In that
particular case, revving the command enabled atomic revprop deletions.)
Daniel
[1] Not including alphas / betas / RCs.
> --
> Philip Martin | Subversion Committer
> WANdisco // *Non-Stop Data*
Re: client side workaround for svnserve iprops bug
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> How do we fix this mess? We could change the server implementation of
> the protocol to match the documentation, but that would risk breaking
> third party clients.
That's not right, it would likely break all clients. I don't think we
can do that. I think we either change the documentation to match the
release or declare want-iprops an error and remove it.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client side workaround for svnserve iprops bug
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
>> We could declare want-iprops to be an error and remove it, that also
>> risks breaking third party client. We could change the documentation
>> of the protocol to match the released implementation, but that would
>> mean accepting the "? foo" form rather than "(? foo)" and I seem to
>> recall that the later is preferred as it's easier to extend.
>
> Unless the documentation is clearly bogus (and in this case, it's not),
> our implementation should match the documentation. The only problem is
> that older clients might not work with a 1.9 server; but there's not
> much we can do about that, short of accepting both forms of the command,
> and I think that would be worse.
I think the documentation might be bogus. The released 1.8 server
parses get-dir using the format:
"c(?r)bb?l?B"
where "?B" is optional. Implementing the documented format would be:
"c(?r)bb?l(?B)"
but I think that would break old clients that send "l" but do not send
"(?B)", i.e. our clients, as well as breaking third party clients that
try to use the released format.
I suppose we could attempt to "correct" both the documentation and the
implementation to use the format:
"c(?r)bb?l?(?B)"
which would work with our old clients, but that would still break any
third party clients trying to use the released 1.8 format.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client side workaround for svnserve iprops bug
Posted by Branko Čibej <br...@wandisco.com>.
On 19.03.2014 14:17, Philip Martin wrote:
> How do we fix this mess? We could change the server implementation of
> the protocol to match the documentation, but that would risk breaking
> third party clients.
A protocol implementation bug is still a bug. We fix it and describe the
fix in the release notes; we've had other cases of API errata.
> We could declare want-iprops to be an error and remove it, that also
> risks breaking third party client. We could change the documentation
> of the protocol to match the released implementation, but that would
> mean accepting the "? foo" form rather than "(? foo)" and I seem to
> recall that the later is preferred as it's easier to extend.
Unless the documentation is clearly bogus (and in this case, it's not),
our implementation should match the documentation. The only problem is
that older clients might not work with a 1.9 server; but there's not
much we can do about that, short of accepting both forms of the command,
and I think that would be worse.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: client side workaround for svnserve iprops bug
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> I would vote "+1 to backport" right here and now, but I'm a bit confused
> by the patch: it seems the client and server send and expect a bare
> boolean, whereas libsvn_ra_svn/protocol calls for a boolean wrapped in a
> tuple.
>
> (i.e., '( foo bar true ) ' vs '( foo bar ( true ) ) '.
It's ugly. I believe the want-iprops flag was added to the get-dir and
get-file commands before the get-iprops command was added. Our client
now uses get-iprops excusively and never sends want-iprops with get-dir
or get-file. I suspect want-iprops should not have been released.
We have:
- the documented protocol
(? want-iprops:boolean )
- the released server implementation of the protocol
? want-iprops:boolean
- the released behaviour
properties always sent
- the trunk behaviour
properties only sent when want-iprops is true
A third party client could already be using want-iprops with a 1.8
server to get inherited props, even with the broken behaviour.
How do we fix this mess? We could change the server implementation of
the protocol to match the documentation, but that would risk breaking
third party clients. We could declare want-iprops to be an error and
remove it, that also risks breaking third party client. We could change
the documentation of the protocol to match the released implementation,
but that would mean accepting the "? foo" form rather than "(? foo)" and
I seem to recall that the later is preferred as it's easier to extend.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client side workaround for svnserve iprops bug
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Mar 18, 2014 at 19:42:29 +0100:
>
>
> > -----Original Message-----
> > From: Philip Martin [mailto:philip.martin@wandisco.com]
> > Sent: dinsdag 18 maart 2014 19:32
> > To: dev@subversion.apache.org
> > Subject: client side workaround for svnserve iprops bug
> >
> > philip@apache.org writes:
> >
> > > Author: philip
> > > Date: Tue Mar 18 12:57:22 2014
> > > New Revision: 1578853
> > >
> > > URL: http://svn.apache.org/r1578853
> > > Log:
> > > Make svnserve recognise when the client does not want inherited
> > > properties. This fixes a performance problem with 1.8 servers
> > > sending too much data.
> >
> > An implementation bug causes svnserve to mishandle the nominally
> > optional want-iprops flag for get-dir and get-file with the result that
> > the server keeps sending properties even when the client doesn't want
> > them, this can be a serious performance regression. (The want-iprops
> > flags was documented/released with 1.8 but isn't used by the released
> > client which uses get-iprops instead.) The problem is fixed on trunk
> > and nominated for 1.8 with 1578853.
> >
> > We could add a client-side workaround for buggy servers, see the patch
> > below. We could do this for trunk, 1.8, even 1.7. Is it a good thing
> > to do?
>
> If think affected users should upgrade (or downgrade) their server to a not
> affected server.
>
> If it the performance was a real show stopper we would have known about this
> problem months earlier.
>
It's a small patch that's unlikely to break anything (regardless of
server version) and may make some users' lives easier. Seems to me like
a good backport candidate.
> For 1.8 a few more user reports can convince me that it would be useful to
> add it, but for svnserve it would be easier to fix a single server than all
> the clients. (It isn't that connected to all kinds of other modules as
> mod_dav_svn).
>
> Adding a protocol changing patch to 1.7 to improve performance against a
> specific set of 1.8 servers... I don't think we should go that route. In
> that case we could better spend our time backporting many of the far more
> important move bugs in 1.8 (several that can corrupt wc.db) that were fixed
> months ago from trunk... Or the ra_serf fixes...
Don't let the perfect be the enemy of the good. Backporting an ra_svn
bugfix is an improvement. If people want to backport ra_serf bugfixes,
that's great... but the fact that no one backported ra_serf fixes isn't
really a good reason to block ra_svn fixes.
---
I would vote "+1 to backport" right here and now, but I'm a bit confused
by the patch: it seems the client and server send and expect a bare
boolean, whereas libsvn_ra_svn/protocol calls for a boolean wrapped in a
tuple.
(i.e., '( foo bar true ) ' vs '( foo bar ( true ) ) '.
RE: client side workaround for svnserve iprops bug
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: dinsdag 18 maart 2014 19:32
> To: dev@subversion.apache.org
> Subject: client side workaround for svnserve iprops bug
>
> philip@apache.org writes:
>
> > Author: philip
> > Date: Tue Mar 18 12:57:22 2014
> > New Revision: 1578853
> >
> > URL: http://svn.apache.org/r1578853
> > Log:
> > Make svnserve recognise when the client does not want inherited
> > properties. This fixes a performance problem with 1.8 servers
> > sending too much data.
>
> An implementation bug causes svnserve to mishandle the nominally
> optional want-iprops flag for get-dir and get-file with the result that
> the server keeps sending properties even when the client doesn't want
> them, this can be a serious performance regression. (The want-iprops
> flags was documented/released with 1.8 but isn't used by the released
> client which uses get-iprops instead.) The problem is fixed on trunk
> and nominated for 1.8 with 1578853.
>
> We could add a client-side workaround for buggy servers, see the patch
> below. We could do this for trunk, 1.8, even 1.7. Is it a good thing
> to do?
If think affected users should upgrade (or downgrade) their server to a not
affected server.
If it the performance was a real show stopper we would have known about this
problem months earlier.
For 1.8 a few more user reports can convince me that it would be useful to
add it, but for svnserve it would be easier to fix a single server than all
the clients. (It isn't that connected to all kinds of other modules as
mod_dav_svn).
Adding a protocol changing patch to 1.7 to improve performance against a
specific set of 1.8 servers... I don't think we should go that route. In
that case we could better spend our time backporting many of the far more
important move bugs in 1.8 (several that can corrupt wc.db) that were fixed
months ago from trunk... Or the ra_serf fixes...
Bert