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
