On 11/05/2016 05:15 PM, Daniel Shahaf wrote: > 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?
Ack. > (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. Did you mean "we might not need a knob to *en*able the cache"? If "calculating the SHA1 is ~10x as fast as actually parsing the file", then performance of disabling cache is always worse. (Ok, except in the case of a modified file, where it adds 10% overhead. But, it's safe to assume that the relative change rate is very low.) Anyway, having a method that provides correctness with a 10% performance penalty (and only on this little step), is imho prefered to having an extra options that a user needs to have an opinion about. I don't see noticable performance issues at all while reparsing the little files every time right now (how big/complex is the average authz file?) on large operations, consisting of many http requests to do checkouts/commits etc. Especially not compared to using htgroup, which feels like it gets into some O(2^n) horror when you put a user into too many groups. >> 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.) Let httpd proxy to itself using keepalive on the backend and do multiple actions? Gheh, but, very tricky, since you don't control the behaviour. But, all of this is a bit ridiculous, since the complexity of the test will vastly overshadow the complexity of the feature being tested, which means there's a bigger chance of having bugs in the test code than having bugs in the feature itself. > 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) So, my general feeling is... Let's forget about this, and +1 for the sha1 cache invalidate way. (hm, complexity of the test still stands, if you would want to test the cache invalidation behaviour...) -- Hans van Kranenburg
