On 02.05.2017 23:53, Alex Rousskov wrote:
AFAICT, r14752 deals with transactions originated by client requests and
broken client connections. Even if all those transactions are fully
covered, there are other transactions (e.g., ESI and Downloader
requests) that may or may not be covered (and seem to be completely
unrelated to r14752 changes!). Is there some kind of common interface
that essentially requires all transactions to have ALE now?
I have not found any ALE manipulations inside ESI and Downloader
code, so that is probably unrelated.
ClientHttpRequest is the object used in transactions, (including
Downloader and ESI code). This object owns its ALE, which is
created in the constructor. I have not found 'common interface'
you are talking about, but if all Squid transactions require
ClientHttpRequest object (which is probably true) then they surely
will have the corresponding ALE.
Normally it is stored in
ClientHttpRequest::al. However, I see that many ACLFilledChecklist
objects does not initialize their ACLFilledChecklist::al (assigning from
the transaction ALE). That works for now in most cases, probably
because there is the only ACLExternal which needs it and overrides
requiresAle() to return true. On the other hand, the lack of proper
ACLFilledChecklist::al initialization makes impossible correct using
ALE component in our 'has' ACL.
Thank you for researching this. Does the following summary accurately
reflect your findings as far as the current code is concerned?
"The current ACL-driven code may feed an ACLFilledChecklist object
without ALE to an (ACLExternal) ACL that requires ALE."
Yes.
Evidently, if we fix this (e.g., with mandatory initializing
ACLFilledChecklist::al in its constructor), we would not need to
check ALE component either.
The question is whether we can "fix this" (i.e., guarantee correct ALE
presence when ALE is needed) _quickly_. I doubt that all
ACLFilledChecklist objects can easily reach the right ALE from their
constructing context. I am worried that fixing this will turn into a
serious, large project. Are you sure it will not?
I agree that it is possible large project, because I am not
sure that every ACL checking context have its ALE near at hand.
If you are not sure, then I suggest keeping "has ALE" support.
... and have our new 'has' ACL broken from the very beginning too.
As I tried to explain earlier, how can we rely on
ACLFilledChecklist::hasAle(), if the ALE itself have not been provided to
ACLFilledChecklist before?
We can
easily make "has ALE" a no-op when/if the code is fixed to always have
access to ALE when ALE is needed.
I see no reason to resist supporting "has ALE" if Squid support for ALE
is currently broken and cannot be fixed easily/quickly. If you see flaws
in this logic, please point them out. I may be missing something.
Eduard.
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
