squid
squid copied to clipboard
squid-cache
AsyncJob-protect and reduce ICAP DNS lookup call chain
@kinkie, another strange Jenkings failure, but this one looks different. I failed to quickly find anything clearly relevant for "Error 141" but I am not even sure which program produces that error. I can only speculate that some low-level instability (or perhaps the usual cache corruption?) was introduced into the test nodes.
09:14:23 make[5]: *** [Makefile:5854: tests/testMath.o] Error 141
@kinkie, another strange Jenkings failure, but this one looks different. I failed to quickly find anything clearly relevant for "Error 141" but I am not even sure which program produces that error. I can only speculate that some low-level instability (or perhaps the usual cache corruption?) was introduced into the test nodes.
09:14:23 make[5]: *** [Makefile:5854: tests/testMath.o] Error 141
I see that the same PR built successfully immediately afterwards. I have no signal about what it might have been. Even the error 141 which I suspect to be the exit code from clang, libtool or ccache, doesn't really have any specific meaning that I could find
why Optional is necessary here
You are right that we could reach the same goal with the default constructed Ip::Address object (it lacks isEmpty(), though) but in this case we do not need to create (and store) the Address object at all! That is one of Optional's purposes - avoid wasting resourses on constructing default objects (especially large ones).
I do not see why Optional is necessary here.
We should use Optional here because Optional<X> is the overall best way to represent absence of a stack-allocated X.
IP already assigns a special value (of 0.0.0.0 or ::) that means unknown/unspecified. That is the value of an default constructed Ip::Address object.
Yes, Ip::Address has many "special" values, including the default-constructed value. As you know, those special address values mean different things in different contexts, and developers struggle with guessing which special Ip::Address value is the right special value for the given context. We can spend hours discussing Ip::Address and ipcache_nbgethostbyname() flaws, but we do not have to do it here: The official ICAP (and ConnOpener) code relevant to this PR is not written to recognize any of those special Ip::Address values and should not be rewritten to recognize them (because there is now a much better way to relay and detect the lack of an IP address in this context).
I do not see why Optional is necessary here.
We should use Optional here because
Optional<X>is the overall best way to represent absence of a stack-allocatedX.
Except when x has its own built-in indicator for absence. In that case both the Optional test AND the type-specific test need to be checked in the receiver. Making the Optional test redundant / bloat.
IP already assigns a special value (of 0.0.0.0 or ::) that means unknown/unspecified. That is the value of an default constructed Ip::Address object.
Yes, Ip::Address has many "special" values, including the default-constructed value. As you know, those special address values mean different things in different contexts, and developers struggle with guessing which special Ip::Address value is the right special value for the given context.
The scope and meanings are defined, even if you do not know them. The value I mentioned above always means unknown/unspecified. What varies by context is how "unknown/unspecified/any" is handled.
The official ICAP (and ConnOpener) code relevant to this PR is not written to recognize any of those special Ip::Address values and should not be rewritten to recognize them (because there is now a much better way to relay and detect the lack of an IP address in this context).
That implementation is buggy. Is this PR not intending to fix that bug?
FYI, The ANYADDR value can be received as explicit values from DNS or hosts file entries. Even if the handlers are relying on Optional - they also need to check the address value and duplicate the logic used when Optional is unset.
We should use Optional here because
Optional<X>is the overall best way to represent absence of a stack-allocatedX.
Except when x has its own built-in indicator for absence.
To save time, I will focus on the in-scope use case: Ip::Address does not have such an indicator and, IMO, should not gain it.
IP already assigns a special value (of 0.0.0.0 or ::) that means unknown/unspecified. That is the value of an default constructed Ip::Address object.
Yes, Ip::Address has many "special" values, including the default-constructed value. As you know, those special address values mean different things in different contexts, and developers struggle with guessing which special Ip::Address value is the right special value for the given context.
The scope and meanings are defined ... The value I mentioned above always means unknown/unspecified. What varies by context is how "unknown/unspecified/any" is handled.
What you said does not contradict what I said, so I am not sure why you are saying this and, to save time, I will not discuss my disagreements with your statement quoted above.
The official ICAP (and ConnOpener) code relevant to this PR is not written to recognize any of those special Ip::Address values and should not be rewritten to recognize them (because there is now a much better way to relay and detect the lack of an IP address in this context).
That implementation is buggy. Is this PR not intending to fix that bug?
No, this PR does not fix that bug. This PR makes a synchronous job call asynchronous, addressing an old TODO. Nothing more. And I bet we disagree on where that bug is located (more on that below)!
Even if the handlers are relying on Optional - they also need to check the address value and duplicate the logic used when Optional is unset.
This part of the debate is outside this PR scope. Most likely, the correct long-term solution for ANYADDR is for DNS resolution code itself to treat special values specially (by default), protecting all ignorant callers (rather than hoping that every caller will remember to check for all those special values). At the very least, that should probably be the default behavior of that resolution code because that is what most (if not all) callers want/need.
Clearing for merge but redoing all tests due to a Jenkins "illegal instruction" failure during the last round of staged commit tests.
The illegal instruction errors were coming out of ccache. I haven't seen them in a while, and I have added a "retry without ccache in case of failure" fallback, it should be safe to retry now Removing the M-abandoned-staging-checks label, hopefully anubis will retry then
Removing the M-abandoned-staging-checks label, hopefully anubis will retry then
Just FYI: That label does not affect commit staging and, hence, staged commit retesting. Anubis is simply telling you that it knows that the last staged commit results are not usable anymore. In this case, they are not usable because the PR branch has changed. Anubis will stage this PR after/if PR branch tests succeed. See Anubis labels documentation for more info.