[squid-dev] [PATCH] renaming StoreEntryStream to PackableStream

[Alex Rousskov]

On 08/22/2015 04:18 AM, Amos Jeffries wrote:
> On 22/08/2015 5:44 a.m., Alex Rousskov wrote:
>> On 08/21/2015 04:38 AM, Amos Jeffries wrote:
>>> So PackableStream is really just a rename of StoreEntryStream BUT with
>>> some implicit new properties from the underlying type change:
>>>
>>> * lack of Store.h dependency
>>>
>>> * ability to stream into a MemBuf if its creator desires that


> Packable does not flush().

Any streambuf, including Packable, must either:

A) support on-demand flushing or
B) flush on each append (essentially making on-demand flushing a no-op)

Since the scope of your changes is StoreEntryStream renaming, and
StoreEntryStream supports (A), (A) is the correct choice for your patch.
There are other reasons why (A) may be better, but keeping changes in
scope should be sufficient.

Packable kids may have kid-specific flush() implementation, of course.
However, within your patch scope, StoreEntry (one of the existing
Packable kids) implementation of flush() should remain unchanged.


> I think it is fixing a previous bug. The Action caller code using
> StoreEntryStream is (still) using buffer() and flush() explicitly to
> prevent lots of small N-byte writes to FD.
> 
> The flush() in StoreEntryStream was completely disabling buffer() state
> despite that explicit use by Action. So the StreamBuf was *permenently*
> disabling Actions intended use of buffer() at the first use of sync().

We can discuss how to fix/improve/optimize this, but it is a complicated
discussion. Complicated fixes/improvements/optimizations are outside
your patch scope. If you think they are important to do soon, let's
discuss them after this email thread is closed.


> If we add flush() to the Packable API I believe we should add buffer()
> and bool isBuffering() const; as well to prevent that type of thing
> happening again in future.

Let's discuss how to optimize the use of flushing after PackableStream
(with flushing support matching current trunk code) is committed.


> BUT. I'm not seeing any current usefulness to it being part of Packable
> API outside of keeping that previously hidden issue in StoreEntryStream.
> Removing/fixing the permanence of the un-buffering breaks bug
> compatibility too.

> Do you see any solid reason why we actually would have to flush() a
> Packable?

There are several, but here is a brief summary of the most important
one: Changing how flushing works, what it means, or when it is used is a
complicated decision outside your patch scope.



>>> +    PackableStream stream(*sentry);
>> ...
>>>      stream.flush();
>>
>> How would that flush() call reach "sentry" if PackableStreamBuf never
>> flushes its buf_?

> stream flush() is not about flushing the sentry AFAIK.

It had that effect before your changes AFAIK.


> It is about ensuring the stream state is all in the buffer memory.

ostream::flush() ensures that previously appended characters get to the
so called "controlled sequence". We can define what "controlled
sequence" is, but REdefining controlled sequence is a complicated
decision outside your patch scope. The current definition should be
preserved, which requires flush()ing in sync() and propagating that
flush() call to StoreEntry when Packable controls StoreEntry.


>>> +        anEntry->lock("test");
>>> +        PackableStream stream(*anEntry);
>> ...
>>> +        delete anEntry; // does the unlock()
>>
>> This leads to a dangling reference in the "stream" object. Please
>> consider narrowing the "stream" scope so that it is destroyed before you
>> delete anEntry.
> 
> The alternatives lead to segfault accessing a raw-pointer that no longer
> exists.

Regardless of the specific problems of specific solutions, the patch
needs to be fixed to avoid dangling pointers.

FWIW, I do not see how destroying the stream object earlier would result
in a dangling pointer access, but perhaps I am missing something or you
are discussing some other alternative. These details are not important
as long as the patch is fixed.


> I thought explicit was relevant to raw-ptr ctor where NULL/0 casting had
> integer related problems. Not for references with type checking.

The "explicit" mark is relevant to all single-parameter constructors
because without that marking, they essentially become implicit type
conversion operators (e.g., converting from Packable to
PackableStreamBuf or from StoreEntry to PackableStream).

Alex.