Re: [PATCH] ecap canonical url invalidation during reqmod

On 01/17/2011 03:34 AM, Amos Jeffries wrote:
> On 17/01/11 21:14, Leonid Evdokimov wrote:
>>> Can you explain why this can't simply always do
>>> safe_free(theMessage.canonical) before re-parsing?
>>
>> I've done this patch to be as non-invasive regarding current logic as
>> possible, so I replicated the logic from ICAP code to eCap code.
>>
>> I think, that the really proper place to
>> safe_free(theMessage.canonical) is marked with FIXME line in my patch
>> - but I'm not 100% sure about that point .
>
> I think its a no for there. The HttpRequest object there should be empty
> anyway at that point.
>
>>
>> Anyway, the patch is required to fix a bug when precache ecap service
>> can't properly rewrite URL (url is rewritten but `canonical` value is
>> stale so request to disk-cache is done with incorrect URL) and it
>> fixes it.
>
> Oh, thanks for pointing ICAP out. The usage in there and elsewhere seems
> a bit wrong. The canonical field should not be filled or copied
> directly, but always generated by urlCanonical(request_object).
>
> The way urlCanonical() works its safe to erase the cached version at any
> point. Just slows Squid down re-calculating it on next use. There may be
> code not using it right though.
>
> I think we should actually do it in url.cc. Removing request->canonical
> at the start of urlParse() and (maybe) setting it in urlParseFinish().
> This will resolve both eCAP and ICAP.

If we can and should always use urlCanonical() to form a new canonical
URL, then we should do that, of course. We should probably hide the
canonical data member itself then, so that no code tries to calculate it
directly.

Similarly, we should check that whenever the url member changes, the
canonical data member is reset. The FIXME-marked code appears one such
place, but perhaps I am wrong.

Thank you,

Alex.
Received on Tue Jan 18 2011 - 01:12:37 MST