Re: mod_authz_svn and caching

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