[squid-dev] [PATCH] Bug 3875 MimeIcon error handling

[Amos Jeffries]

On 2/06/2015 12:23 p.m., Alex Rousskov wrote:
> On 06/01/2015 05:33 PM, Amos Jeffries wrote:
>> On 21/05/2015 3:22 a.m., Amos Jeffries wrote:
>>> This is an attempt to improve the MimeIcon reliability when filesystem
>>> I/O errors or others cause the icon data to not be loadable.
>>>
>>> The loading process is re-worked to guarantee that once the
>>> MimeIon::created callback occurs it will result in a valid StoreEntry in
>>> the cache representing the wanted icon.
>>>  * If the image can be loaded without any issues it will be placed in
>>> the cache as a 200 response.
>>>  * If errors prevent the image being loaded or necessary parameters
>>> (size and mtime) being known a 204 object will be placed into the cache.
>>>
>>> I have selected 204 as the representation of errors since the bug is not
>>> in the clients request (eliminating 400, 404, etc), a 500 would be
>>> revealing details about server internals unnecessarily often and incur
>>> extra complexity creating the error page. 204 also avoids needing to set
>>> and emit Content-Length header, and just enough different from 200 to
>>> use as a special-case test.
>>>
>>>
>>>
>>> NP: It started with just correcting the errno usage, but other bugs
>>> promptly started appearing once I got to seriously testing this load
>>> process. So far it fixes:
>>> * several assertions resulting from StoreEntry being left invalid in
>>> cache limbo beween created hash entries and valid mem_obj data.
>>> * on startup 6+ repeated attempts to load absent icons files which dont
>>> exist in the filesystem.
>>> * buffer overfow on misconfigured or corrupt mime.conf file entries
>>> * incorrect debugs messages about file I/O errors
>>> * large error pages delivered when icons not installed (when it doesnt
>>> assert from the StoreEntry)
> 
> 
>> If there are no objections I will appy this tomorrow.
> 
> Using a successful 2xx code to indicate an error sounds like a mistake
> to me. Moreover, 204 responses have special framing rules that increase
> the chance of them being misinterpreted by poorly written clients.

Its not a protocol error. It may or not be a configuration error on the
part of the sysadmin though.

The proxy has been explicitly configured to "know" this icon URL and has
no content loaded from disk to deliver for it. As far as the client and
any downstream intermediaries are concerned that is the relevant info.

> 
> If there are no better error codes, I suggest using a 503 Service
> Unavailable response. If it is difficult to adjust the code to serve an
> existing error message in this context, then just set Content-Length to
> zero or close connection, whichever is easier.

503 is not appropriate. That implies Connection:close and client being
diverted to another server/proxy (if using PAC or reverse-proxy). The
issue here has nothing to do with the connection or future usability of
the proxy producing this response.


If we are going to advertise these at all to the client, then 500 is the
only applicable registered 5xx status. Since it is also technically an
"Internal Error" while loading the icon.

I dont believe there is anything worth informing the client about
though. If the admin cares about the icons and notices them absent the
errors are logged at the time they actually occur, the cached icon object.

> 
> You have mentioned that 204 "avoids needing to set [...] Content-Length
> header", but AFAICT, the content length is just a setHeaders() parameter
> which can be zero. In fact, your patch uses it! It looks like that part
> of the argument against a true error response is invalid.

FWIW, "set and emit". Saving a few bytes in the headers and allows the
response to be cacheable by default. Both being incidental to the
problems I'm fixing, but potentially useful in general if the URL gets
hit a lot.


> 
> If you are sure your solution is better than doing nothing, please do
> not interpret the above as an objection.
> 
> 
> If you decide to commit this, please do include your NP paragraph to
> document what kind of "reliability" problems this patch fixes.
> 

Okay.

Amos