Re: [PATCH] drop Request-Range header support

On 02/28/2014 12:28 AM, Amos Jeffries wrote:

> Please look at what decideIfWeDoRanges() already does to the server traffic.
> Under your proposal to ignore Request-Range and treat Range normally

That was not my proposal.

> Squid will newely become a client emitting only Request-Range whenever
> range_offset_limit exceeded or connection-auth is used.

That would be wrong, of course, but I have not proposed that.

> Under my proposal it would never emit Request-Range and treat Range like
> it does now normally. Except not being overridden if Request-Range
> follows it. The result is that the Range feature behaviour should happen
> as per the spec, without unexpected range responses appearing from
> servers (and from older Squid installs) taking the Request-Range values
> as their basis.

If you are willing to bet that no currently working client relies on
Request-Range handled as Range, it is OK to delete and ignore the
Request-Range header. This is not what I recommend doing, but, again, I
am not objecting to such a change if others think it is the best way
forward.

>> Range is an end-to-end
>> header. No OPTIONAL feature would work if proxies start dropping the
>> headers related to it!
>
> Agreed on that last sentence. Which is why the _original_ proposal was
> only to discard the conflicting non-standard and apparently *rare*
> legacy header (Request-Range). Or to drop the lot when there is a conflict.

Forgive me for being annoying, but your original proposal was different
from how you remember it. You proposed two things:

  A) erase Request-Range header without processing
  B) treat multiple Range headers as an error and dropping them all

Perhaps you meant something else, but that is how I interpret what you
have written.

My initial response focused on (B). I noted that it may be a good idea
to exclude multiple identical headers from the drastic effects of (B).

The same value conflict-based approach can and should be applied to all
*Range headers at once, of course, but my initial response focused on
(B) because that case is more likely to cause trouble and if we cannot
agree there, there is little point in adding Request-Range into the mix.

> At present mapping a non-standard legacy Request-Range header into an
> server Range request as if it were just "Range:". Overriding any actual
> Range header which preceeded it (and being overridden if its preceeding
> a Range).

I think we are all in agreement that the present code is bad and can be
improved.

>>> There are also later supporting statements with explicit MAY for server
>>> ignoring the headers, and proxies dropping headers with unknown units.
>>> All of which implies strongly the intent of this.
>>
>> I saw those statements. I do not think they imply that proxies can
>> remove Range. If the intent was to say that proxies can drop Ranges (not
>> ignore, but remove them!), the specs are buggy and should be adjusted to
>> say that explicitly.
>
> This bit seems quite explicit about proxy being allowed to drop/discard
> Range when appropriate:
>
> " A proxy MAY discard a Range
> header field that contains a range unit it does not understand.
> "

Sure, but that rule has nothing to do with this discussion IMO. It is
specific to a single field with an unknown range unit while we are
discussing legacy header and multiple fields (the unit analysis comes
later or never, depending on the approach taken).

> The header list unfolding is supposed to happen forst before
> interpretation. So two Range headers are *supposed* by the message
> syntax to concatenate with a ','.

No. Range header does not have a #list field value syntax and so the
server must not concatenate its values (this is RFC 2616 logic, please
let me know if HTTPbis changes that).

> ... I am reading the OPTIONAL as implying we can pretend not to support
> "bytes" unit on these broken requests and discard the invalid field-value.

I do not think we should debate inventive interpretations of the
protocol language. I think we all agree that Squid _can_ drop invalid
Range and/or Request-Range headers.

The only significant difference in opinion, AFAICT, is that you think it
is a good idea to drop:

  Range: bytes=0-10
  Range: bytes=0-10

or

  Request-Range: bytes=0-10
  Range: bytes=0-10

and other combinations of *identical range values* while I think those
cases should be treated specially (for interoperability sake and because
it is clear what the sender intended).

> The feature is entirely dependent on this first header being delivered.
> Breakage risk only starts occuring if the response headers are fiddled
> with. If this request header is never delivered at all the feature is
> simply not used. HTTP continues working fine.

HTTP continues to work fine but we may still get complaints that clients
"stop working" when and only when going through Squid. I am not arguing
that a compliant client may send multiple identical Range headers. I am
not arguing that a client may require a range response. I am just saying
that needlessly preventing a poorly written client from working is not
in our best interest. If we can safely and without significant pains
allow that client to continue to work when going through Squid, we
should do that (or at least claim that as a long-term goal and make
smaller steps towards that).

>>> Ignoring conflicting Range and Content-Range may be hard.
>>
>> Ignoring them is not hard. Just keep HttpRequest::range pointer NULL and
>> Squid will ignore them. Please note that I am advocating ignoring all
>> *Range headers if at least two of them conflict. I am not advocating
>> ignoring just some of the *Range headers in the request. If one of them
>> is broken or any two are inconsistent, we should ignore all of them.
>>
>
> By the time Content-Range is received and conflicts we have already
> *not* ignored Range in its entirety. One Range header is being managed,
> the others have been ignored and we have no idea if the ignored one(s)
> is what the server response is about.

Now you are arguing about detecting conflicting Range and Content-Range
headers. *Ignoring* them is not hard -- the current APIs already support
that. *Detecting* them does require more work than the current code does.

>>> How about a mid-way solution (C). Moving the Request-Range logics into
>>> an undefined "#if USE_OBSOLETE_REQUEST_RANGE" wrapper and waiting for
>>> complaints?
>>
>> Your call. As I said, I do not object to removal of that code, whether
>> that means #ifdefing it, commenting it out, or removing it completely.
>>
>
> But you are objecting to ignoring Request-Range?

I do not think it is a good idea (for interoperability reasons already
discussed), bit I do not object to that if others think it is a good idea.

Please do not attribute one of your patches to me. It has nothing to do
with my suggestions.

Thank you,

Alex.
Received on Fri Feb 28 2014 - 17:54:23 MST