Hans van Kranenburg wrote on Sat, Nov 05, 2016 at 16:39:44 +0100:
> On 11/05/2016 11:44 AM, Stefan Fuhrmann wrote:
> > Looking at your workaround, I think the cleanest solution would be to
> > introduce an AuthzSVNDisableCache option.
I'm not sure a knob to _disable_ caching is the right approach. So long
as using the cache trades off correctness for performance, shouldn't the
cache be off by default, with a knob to enable it? I.e., shouldn't the
authz code be conservative and default to prefering correctness over
performance?
(However, this point may be moot; see below.)
> > On 05.11.2016 00:19, Hans van Kranenburg wrote:
> >> I can give it a shot to prepare a patch to do this, if wanted.
> >
> > Patches are always welcome. I think adding AuthzSVNDisableCache
> > should be easy enough for "giving it a shot".
>
> I guess I have to target the development (to be 1.10) code for the
> patch?
You would, yes. We don't add new features in patch releases (1.x.y
with y>0).
> I also guess that the cache invalidation change will have to lead
> to deprecation/removal of the option again shortly thereafter, causing
> extra cruft in the code.
That is a valid point: if cache invalidation is performant, we might not
need a knob to disable the cache.
> So, does it make sense at all? I'm using Debian on the servers, with 1.8
> now, and 1.9 in the next stable release, so I'll have to roll my own
> packages anyway, for which the quick patch is good enough.
(For the list archives: the 'quick patch' updates get_access_conf() to
remove the CACHE_KEY computation and always take the «access_conf ==
NULL» codepath.)
> I also guess that ideally the tests need to be updated with cases which
> show that authz file changes while doing requests are not seen when
> caching is enabled and doing multiple requests uwing HTTP/1.1 keepalive
> on a single connection, while they *are* seen when AuthzSVNDisableCache
> is enabled. From what I see so far, this is not a trivial undertaking.
The catch is, how to write code that commits twice to a single
repository over a single connection. I don't think the command-line
client lets you do that. Given that we want to test both svnserve and
mod_dav_svn, using httplib directly would be suboptimal. I think that
leaves (a) write a C test, (b) write a C helper tool for doing multiple
commits over a single connection and write a Python test using that.
(Example: atomic-ra-revprop-change.)
As to changing the value of AuthzSVNDisableCache: the test framework
doesn't let you do that; it presumes an httpd is already running. But
the test suite could launch the server with cache enabled, and provide
a harness option to run with cache disabled, similar to the handling of
the 'short-circuit' option.
(I assume the parsed file's cache lifetime is per-connection in svnserve too)
Cheers,
Daniel
Received on 2016-11-05 17:15:51 CET