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

[Alex Rousskov]

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.

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.

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.

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.


Thank you,

Alex.