Call Client methods synchronously in FetchDataLoader if the first read is Done or error

Call Client methods synchronously in FetchDataLoader if the first read is Done or error

Preparation for https://codereview.chromium.org/1192913007/.
Handles like createDoneDataConsumerHandle() or createUnexpectedErrorDataConsumerHandle() can be created temporarily, and the handles and associated FetchDataLoader might be difficult to be kept alive.
This CL avoid the problem of keeping alive such handles by completing load for such handles synchronously in start().

BUG=480746

[hiroshige, 2015-07-01 13:17:41]

hiroshige@chromium.org changed reviewers:
+ yhirano@chromium.org

[hiroshige, 2015-07-01 18:57:01]

PTAL.

[yhirano, 2015-07-02 06:33:18]

Can you tell me why you need the synchronous reading?

> Handles like createDoneDataConsumerHandle() or
createUnexpectedErrorDataConsumerHandle() can be created temporarily, and the
handles and associated FetchDataLoader might be difficult to be kept alive.

I don't understand why the synchronous reading solves the problem.

[hiroshige, 2015-07-02 08:22:34]

On 2015/07/02 06:33:18, yhirano wrote:
> Can you tell me why you need the synchronous reading?
> 
> > Handles like createDoneDataConsumerHandle() or
> createUnexpectedErrorDataConsumerHandle() can be created temporarily, and the
> handles and associated FetchDataLoader might be difficult to be kept alive.
> 
> I don't understand why the synchronous reading solves the problem.

In https://codereview.chromium.org/1192913007/ (Patch Set 35),
BodyStreamBuffer::startLoading() (in BodyStreamBuffer.cpp) calls
FetchDataLoader::start() and creates a reference from BodyStreamBuffer to
FetchDataLoader to keep alive the loader while loading.
However, the BodyStreamBuffer can be temporary object created in
Body::ReadableStreamSource::createDrainingStream() (e.g. if
m_stream->stateInternal() == ReadableByteStream::Closed), so the loader can be
garbage collected along with BodyStreamBuffer before loading completes.

Another candidate is to let createDrainingStream() to make a reference from Body
to the returned BodyStreamBuffer (Patch Set 36).
We can proceed to review of https://codereview.chromium.org/1192913007/ and then
get back to this CL later.

[yhirano, 2015-07-03 09:41:42]

On 2015/07/02 08:22:34, hiroshige wrote:
> On 2015/07/02 06:33:18, yhirano wrote:
> > Can you tell me why you need the synchronous reading?
> > 
> > > Handles like createDoneDataConsumerHandle() or
> > createUnexpectedErrorDataConsumerHandle() can be created temporarily, and
the
> > handles and associated FetchDataLoader might be difficult to be kept alive.
> > 
> > I don't understand why the synchronous reading solves the problem.
> 
> In https://codereview.chromium.org/1192913007/ (Patch Set 35),
> BodyStreamBuffer::startLoading() (in BodyStreamBuffer.cpp) calls
> FetchDataLoader::start() and creates a reference from BodyStreamBuffer to
> FetchDataLoader to keep alive the loader while loading.
> However, the BodyStreamBuffer can be temporary object created in
> Body::ReadableStreamSource::createDrainingStream() (e.g. if
> m_stream->stateInternal() == ReadableByteStream::Closed), so the loader can be
> garbage collected along with BodyStreamBuffer before loading completes.
> 
> Another candidate is to let createDrainingStream() to make a reference from
Body
> to the returned BodyStreamBuffer (Patch Set 36).
> We can proceed to review of https://codereview.chromium.org/1192913007/ and
then
> get back to this CL later.


I think now I understand, so let me confirm.

We need to load data from a draining stream using FetchDataLoader.
DrainingBodyStream has a BodyStreamBuffer reference, and BodyStreamBuffer has a
FetchDataLoader reference. That reference keeps the loader alive while loading.
That means one of the draining stream, the body stream buffer and the loader
should be kept while loading. In usual cases the condition holds because Body
has its body stream buffer. However, when the stream is closed or errored,
createDrainingStream creates a new body stream buffer and creates a draining
buffer with it. In this case, they are not reachable from the Oilpan GC.
But in this case we can know that the body stream is closed or errored
immediately (*). That means reading synchronously solves the problem - an
"unfinished" stream is always kept.

Is my understanding correct? If so, I don't like this approach because relying
on (*) sounds fragile to me.

[hiroshige, 2015-07-05 06:39:04]

> We need to load data from a draining stream using FetchDataLoader.
> DrainingBodyStream has a BodyStreamBuffer reference, and BodyStreamBuffer has
a
> FetchDataLoader reference. That reference keeps the loader alive while
loading.
> That means one of the draining stream, the body stream buffer and the loader
> should be kept while loading.
Yes.
(But we don't have to keep DrainingBodyStream alive -- DrainingBodyStream can be
destructed just after FetchDataLoader start)

> In usual cases the condition holds because Body
> has its body stream buffer. However, when the stream is closed or errored,
> createDrainingStream creates a new body stream buffer and creates a draining
> buffer with it. In this case, they are not reachable from the Oilpan GC.
Yes.

> But in this case we can know that the body stream is closed or errored
> immediately (*). That means reading synchronously solves the problem - an
> "unfinished" stream is always kept.
Yes.

> Is my understanding correct? If so, I don't like this approach because relying
> on (*) sounds fragile to me.
Yeah, I also think this is not so clear design because FetchDataLoader and
createDrainingStream() interacts in a very specific way.

Do you have any suggestions for ensuring loading in such cases?

[yhirano, 2015-07-06 03:27:29]

On 2015/07/05 06:39:04, hiroshige wrote:
> > We need to load data from a draining stream using FetchDataLoader.
> > DrainingBodyStream has a BodyStreamBuffer reference, and BodyStreamBuffer
has
> a
> > FetchDataLoader reference. That reference keeps the loader alive while
> loading.
> > That means one of the draining stream, the body stream buffer and the loader
> > should be kept while loading.
> Yes.
> (But we don't have to keep DrainingBodyStream alive -- DrainingBodyStream can
be
> destructed just after FetchDataLoader start)
> 
> > In usual cases the condition holds because Body
> > has its body stream buffer. However, when the stream is closed or errored,
> > createDrainingStream creates a new body stream buffer and creates a draining
> > buffer with it. In this case, they are not reachable from the Oilpan GC.
> Yes.
> 
> > But in this case we can know that the body stream is closed or errored
> > immediately (*). That means reading synchronously solves the problem - an
> > "unfinished" stream is always kept.
> Yes.
> 
> > Is my understanding correct? If so, I don't like this approach because
relying
> > on (*) sounds fragile to me.
> Yeah, I also think this is not so clear design because FetchDataLoader and
> createDrainingStream() interacts in a very specific way.
> 
> Do you have any suggestions for ensuring loading in such cases?

I like your your fix in https://codereview.chromium.org/1192913007/ better.

[hiroshige, 2015-07-06 04:52:52]

> I like your your fix in https://codereview.chromium.org/1192913007/ better.

OK, then I'll use the fix and close this CL.
Thanks!