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

[Amos Jeffries]

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
>>
>> This patch demonstrates that by doing the upgrade in-place and re-typing
>> all StoreEntryStream objects.
>>
>> It includes some extra polishing of the *StreamBuf logics to remove
>> styling and redundant code issues identified in the previous cycles of
>> discussion. But otherwise is essentially a drop-in replacement.
> 
> 
> Thank you for making a well-scoped change. It makes finding problems a
> lot easier.
> 
> 
>> +    virtual int sync() override {
>>          std::streamsize pending(pptr() - pbase());
>> -
>> -        if (pending)
>> -            theEntry->append(pbase(), pending);
>> -
>> -        theEntry->flush();
>> -
>> +        lowAppend(pbase(), pending);
>>          return 0;
>>      }
> 
> Patched sync() no longer flush()es the entry. This is a significant
> undocumented change and,

"ability to stream into a MemBuf". but I see what you mean.

> AFAICT, a major bug. If I am right (see below
> for specifics), please consider adding a pure virtual Packable::flush()
> method and calling that in the above code _regardless_ of the "pending"
> value, just like the old code used to do.

That was intentional. Packable does not flush().

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().


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.

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?


> 
>> +    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 is about
ensuring the stream state is all in the buffer memory.

flushing the sentry data to the FD is a separate event done by Action later.


> 
>> +        anEntry->lock("test");
>> +        PackableStream stream(*anEntry);
>>  
>>          stream.setf(std::ios::fixed);
>>          stream << 123456 << std::setprecision(1) << 77.7;
>>          stream << " some text" << std::setw(4) << "!" << '.';
>>          stream.flush();
>> -        // flushed at least once more
>> -        CPPUNIT_ASSERT(anEntry->_flush_calls > preFlushCount);
>> +        CPPUNIT_ASSERT_EQUAL(String("12345677.7 some text   !."), anEntry->_appended_text);
>>  
>> +        delete anEntry; // does the unlock()
> 
> Checking the appended text without checking that the entry was flushed
> is not sufficient because the appended text does not know what store
> entry sent to Store clients. It only shows what Store servers appended
> to the entry.
> 

This is a PackableStream API unit test. The stream flush() ensure the
text is in the entry memory. The unit test is testing that it got there
completely and correctly.


The entry flush() ensures the entry text reaches the recipient / store -
which may have pruned from the entry if it is a Transient-like store.
But all that is not relevant to Packable API.

This is why the flush accounting is removed from the test cases. We are
testing only PackableStream use of Packable API here, not extra
StoreEntry API behaviours.

The only relevance entry flush() has is after a buffer() to cleanup
StoreEntry internal state before its deletion. And buffer() is not being
used.

> 
>> +        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.


> 
>> +    PackableStreamBuf(Packable &p) : buf_(p) {}
> 
> If you can make the ctor explicit, please do.
> 
> 
>> +    PackableStream(Packable &p) : std::ostream(0), theBuffer(p) {
> 
> Ditto.
> 

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

Done for both anyway.

> 
>> +    ~PackableStreamBuf() = default;
> 
> I would remove that for simplicity and consistency sake.
> 
> Rule of thumb: Given a set of method kinds that can be marked as default
> or can be explicitly deleted (i.e., copy constructor, destructor,
> assignment operator, etc.), we should either explicitly declare all such
> methods or none at all. None is the right choice when we do not _need_
> to redefine any default methods or explicitly delete any methods.
> 
> The "none" choice applies to both PackableStreamBuf and PackableStream.

Okay. Done.

Amos