net: Make UploadDataStream::Init() asynchronous.

net: Make UploadDataStream::Init() asynchronous. 

Rename the existing synchronous version to InitSync() and add a new asynchronous version, Init().
However, the new asynchronous version is not used yet.
The existing code is changed to use the synchronous version.
ScopedAllowIO used in UploadData::Element::ResetOffset() is removed since it is always called on an IO allowed thread.

BUG=72001, 113669
TEST=net_unittests

[hashimoto, 2012-08-06 08:47:00]

willchan,

Could you review this?

[hashimoto, 2012-08-06 08:52:42]

This change is originally satorux's. http://codereview.chromium.org/9350060/
I took over the issues from him.

[hashimoto, 2012-08-08 03:29:42]

Eric, could you review this change?

[eroman, 2012-08-09 05:29:33]

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data.cc
File net/base/upload_data.cc (left):

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data.cc#old...
net/base/upload_data.cc:124: base::ThreadRestrictions::ScopedAllowIO allow_io;
This removal seems unrelated to this changelist (I don't see any changes made to
CloseSync()), or am I missing something?

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream...
net/base/upload_data_stream.cc:21: void InitInternal(scoped_refptr<UploadData>
upload_data,
UploadData is not threadsafe, however now it is being passed off to a worker
thread and used there. I do not believe this is safe.

(Even just calling AddRef() implicitly by the scoped_refptr<> can be risky
business when done from multiple threads and the base class was RefCounted
rather than RefCountedThreadSafe).

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream...
net/base/upload_data_stream.cc:51: return;
delete this

[hashimoto, 2012-08-09 13:35:02]

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data.cc
File net/base/upload_data.cc (left):

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data.cc#old...
net/base/upload_data.cc:124: base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/08/09 05:29:33, eroman wrote:
> This removal seems unrelated to this changelist (I don't see any changes made
to
> CloseSync()), or am I missing something?

UploadData::ResetOffset() is the only user of
UploadData::Element::ResetOffset(), and UploadDataStream::Init() is the only
user of UploadData::ResetOffset().

|allow_io| here can be removed since UploadDataStream::Init() calls this method
only on IO allowed threads.

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream...
net/base/upload_data_stream.cc:21: void InitInternal(scoped_refptr<UploadData>
upload_data,
On 2012/08/09 05:29:33, eroman wrote:
> UploadData is not threadsafe, however now it is being passed off to a worker
> thread and used there. I do not believe this is safe.
Oh I thought touching UploadData on a worker thread is safe because
UploadData::GetContentLength is doing it.
It seems we have to fix GetContentLength first.

> (Even just calling AddRef() implicitly by the scoped_refptr<> can be risky
> business when done from multiple threads and the base class was RefCounted
> rather than RefCountedThreadSafe).
How about making UploadData RecCountedThreadSafe and adding a class comment "Do
not touch this class while asynchronous operation is in progress" for
UploadData?
It is safe as long as all public methods of UploadData are called only on one
thread (i.e. IO thread) and only private methods touch UploadData on worker
threads.

If we are able to touch UploadData on worker threads, it becomes very easy to
avoid code duplication between synchronous and asynchronous implementations.
(Synchronous implementation should be maintained unless we fix Chrome Frame
radically)

[hashimoto, 2012-08-14 00:34:29]

On 2012/08/09 13:35:02, hashimoto wrote:
> http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data.cc
> File net/base/upload_data.cc (left):
> 
>
http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data.cc#old...
> net/base/upload_data.cc:124: base::ThreadRestrictions::ScopedAllowIO allow_io;
> On 2012/08/09 05:29:33, eroman wrote:
> > This removal seems unrelated to this changelist (I don't see any changes
made
> to
> > CloseSync()), or am I missing something?
> 
> UploadData::ResetOffset() is the only user of
> UploadData::Element::ResetOffset(), and UploadDataStream::Init() is the only
> user of UploadData::ResetOffset().
> 
> |allow_io| here can be removed since UploadDataStream::Init() calls this
method
> only on IO allowed threads.
> 
>
http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream.cc
> File net/base/upload_data_stream.cc (right):
> 
>
http://codereview.chromium.org/10823180/diff/4003/net/base/upload_data_stream...
> net/base/upload_data_stream.cc:21: void InitInternal(scoped_refptr<UploadData>
> upload_data,
> On 2012/08/09 05:29:33, eroman wrote:
> > UploadData is not threadsafe, however now it is being passed off to a worker
> > thread and used there. I do not believe this is safe.
> Oh I thought touching UploadData on a worker thread is safe because
> UploadData::GetContentLength is doing it.
> It seems we have to fix GetContentLength first.
> 
> > (Even just calling AddRef() implicitly by the scoped_refptr<> can be risky
> > business when done from multiple threads and the base class was RefCounted
> > rather than RefCountedThreadSafe).
> How about making UploadData RecCountedThreadSafe and adding a class comment
"Do
> not touch this class while asynchronous operation is in progress" for
> UploadData?
> It is safe as long as all public methods of UploadData are called only on one
> thread (i.e. IO thread) and only private methods touch UploadData on worker
> threads.
> 
> If we are able to touch UploadData on worker threads, it becomes very easy to
> avoid code duplication between synchronous and asynchronous implementations.
> (Synchronous implementation should be maintained unless we fix Chrome Frame
> radically)

ping?

[eroman, 2012-08-15 18:23:10]

Sorry for the delay, I have been overwhelmed with some other stuff!

I will try to respond again later today.

[eroman, 2012-08-21 00:51:18]

Relying on callers invoking it just one at at time seems too fragile. It is also
hard to guarantee this with a refcounted class + cancellation (maybe for
instance someone cancels this job, but the worker thread keeps running, and
simultaneously another user starts another worker thread to touch uploaddata).

I am not really familiar enough with the current state of these classes to give
a better suggestion.

Perhaps willchan/satorux/kinuko can offer better advice regarding using
UploadData from a worker thread and how to split this off!

[willchan no longer on Chromium, 2012-08-21 02:55:45]

Doh! I thought you were going to take care of this for me so I didn't have
to review it! I'll take a look, hopefully tomorrow.


On Mon, Aug 20, 2012 at 5:51 PM, <eroman@chromium.org> wrote:

> Relying on callers invoking it just one at at time seems too fragile. It
> is also
> hard to guarantee this with a refcounted class + cancellation (maybe for
> instance someone cancels this job, but the worker thread keeps running, and
> simultaneously another user starts another worker thread to touch
> uploaddata).
>
> I am not really familiar enough with the current state of these classes to
> give
> a better suggestion.
>
> Perhaps willchan/satorux/kinuko can offer better advice regarding using
> UploadData from a worker thread and how to split this off!
>
>
http://codereview.chromium.**org/10823180/<http://codereview.chromium.org/108...
>