Re: [PATCH] HTTP Parser upgrade

On 06/02/2014 09:45 AM, Amos Jeffries wrote:
> On 21/05/2014 4:04 a.m., Alex Rousskov wrote:
>> On 05/20/2014 05:17 AM, Amos Jeffries wrote:
>>> + explicit Parser(const Parser&); // do not implement
>>> + Parser& operator =(const Parser&); // do not implement

>> Why add these two? The Parser class appears to be perfectly copyable "as
>> is" to me (which is a good thing!). Did I miss any non-copyable fields?

> Those are there because there should only ever be one owner of a Parser
> which also feeds it with bytes to process - the ConnStateData currently.
> It gets passed around via & params or a ref-counted Pointer. If copies
> of the parser itself are being grabbed by other classes something has
> changed with the design and we need to check the code using it.

There are two problems with this reasoning:

* Copying is not about sharing; it is about duplication which is almost
orthogonal to sharing. It is OK to parse copyable objects by reference
when copying is not needed, of course. The existence of such references
is irrelevant when deciding on whether an object is copyable.

* You are making things more complex than they are by ignoring the
encapsulation principle: Parser ownership is external to the Parser
class. Some code somewhere may require a single Parser owner, but the
Parser class can still support copying.

In general, we make classes non-copyable for two reasons:

* It requires a lot of work to implement copying correctly; and/or
* Copying is very expensive performance-wise (including RAM, etc.).

Neither reason applies to Parser AFAICT. Thus, we should keep it
copyable. Being copyable is a good thing!

>>> + RequestParser() : Parser() {}
>>
>> I would also remove the above as unneeded (provided by the compiler).
>>
>
> Removing results in several compile errors "no matching function for
> call to Http::One::RequestParser::RequestParser()"

This means there are still other non-generated constructors present,
preventing the compiler from generating a default constructor.

> I've added unit tests now to check the state on construction is correct
> and found that RequestParser() needs to call clear() itself as the
> parent does not call the right virtual clear().

Constructors do not (and cannot) call kid's methods; they only call
methods of their own class. There are two solutions:

* Use clear() for already constructed (and used) objects only. Implement
proper initialization in each constructor. This is the best overall
approach, but it is sometimes difficult to implement without code
duplication.

* Let the final child constructor and only that constructor call
clear(). This works OK if you only create objects of one child class on
each inheritance branch. For example, you create Http1Parser objects and
no class inherits from Http1Parser.

In both cases, each clear() must call its parent method, of course.

I will try to review this aspect once the new patch is available.

> FYI: I am attempting to make this RFC 7230 compliant (the HTTPbis
> document on HTTP/1 message syntax).

Please make sure Squid still accepts messages it used to accept before
unless rejection is absolutely necessary. Supporting new RFCs should not
come at the expense of supporting real-world traffic (to the extent
possible).

> Is coadvisor able to test that latest criteria yet?

Some. We are working on adding more HTTPbis-specific cases.

HTH,

Alex.
Received on Tue Jun 03 2014 - 14:18:34 MDT