925838 - Fix NetUtil.asyncCopy to do what people think it does (no main thread IO)

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[(dormant account)]

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp should be refactored to 
a) require streams to implement some interface that grantees asyncness(maybe nsIAsyncInputStream?)
b) such an interface would provide async open/close/(flush?)
c) for bonus points we'd copy data using some kernel-level interface like splice() instead of current naive methods

Comment 1

[(dormant account)]

NetUtil.asyncCopy should strongly suggest that it calls open/close rather than let clients do this as most existing code would cancel out any async benefits by calling open/close().

Comment 2

[David Teller [:Yoric] - still alive but not very active]

So, this means changing:
* NS_AsyncCopy() and its clients;
* nsIAsyncStreamCopier.idl;
* NetUtil.jsm

On m-c, the change seems nicely bounded, but I suspect that there will be a number of add-ons affected. Waiting until mxr is back online to find out more.

Comment 3

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> So, this means changing:
> * NS_AsyncCopy() and its clients;
> * nsIAsyncStreamCopier.idl;
> * NetUtil.jsm
> 
> On m-c, the change seems nicely bounded, but I suspect that there will be a
> number of add-ons affected. Waiting until mxr is back online to find out
> more.

Note, I'm ok with 'fixing' this by providing a new method that does right thing(tm) + leaving the old method to do angry warnings. Though making this call do the right thing would be even better if it can be done without bustage.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

If I understand correctly how NS_AsyncCopy works (see bug 925831), the only thing that we can really improve is opening OMT. API-wise, the simplest is probably to let clients pass a nsIURI.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Ah, we should also get rid of the calls to |ioUtil.{input, output}StreamIsBuffered| on the main thread as they can cause main thread I/O.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Possible nsIAsyncStreamCopier API

So, here's a first API draft. The idl version looks rather heavy-handed, but the JS version should be rather nice.

Nathan, Vlad, what do you think of this?

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 820270 [details] [diff] [review]
Possible nsIAsyncStreamCopier API

Review of attachment 820270 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this is OK.  You might want to solicit some feedbacking from a networking peer, too, since they're the ones who should be reviewing this eventually.

::: netwerk/base/public/nsIAsyncStreamCopier.idl
@@ +66,5 @@
> +    const long BUFFERING_UNBUFFERED = 1;
> +    // Stream is known to be unbuffered, i.e. ReadSegments is unimplemented
> +    // (or should not be used).
> +    const long BUFFERING_AUTODETECT = 2;
> +     // Autodetect stream buffering.

Do you have use cases in mind for the BUFFERED and UNBUFFERED cases?  They're not present in the initial changes.

@@ +80,5 @@
> +     * @param aClose Close the stream (off main thread) at the end of
> +     *                copy. This is generally a good idea.
> +     */
> +    void initSourceFromFile(in nsIFile aSource);
> +    void initSourceFromURI(in nsIURI aSource, in long aBuffering);

Is there any reason to specify buffering on nsIURIs, for either the source or the sink?  Presumably we're opening them internally to get the stream, so the user doesn't need to care/know whether buffering is implemented on such streams.

@@ +82,5 @@
> +     */
> +    void initSourceFromFile(in nsIFile aSource);
> +    void initSourceFromURI(in nsIURI aSource, in long aBuffering);
> +    void initSourceFromStream(in nsIInputStream aSource,
> +                              in long aBuffering, in boolean aClose);

For conformity with the existing API of init(), I'd just let aBuffering be a bool here.

@@ +102,5 @@
> +     */
> +    void initSinkFromFile(in nsIFile aSink, in bool aThroughTemporaryFile);
> +    void initSinkFromURI(in nsIURI aSink, in long aBuffering);
> +    void initSinkFromStream(in nsIOutputStream aSink,
> +                            in long aBuffering, in boolean aClose);

The comments for sources apply to sinks too.

@@ +117,5 @@
> +     *        specifies how many bytes to read/write at a time.  this controls
> +     *        the granularity of the copying.  it should match the segment size
> +     *        of the "buffered" streams involved.
> +     */
> +    void initBehavior(in boolean aPickSinkForBufferization,

I think this would be better as a constant: SOURCE_BUFFERED, SINK_BUFFERED, BOTH_BUFFERED(?).  Why do we have to pick between the two?  We should be able to have both.

::: netwerk/base/src/NetUtil.jsm
@@ +83,5 @@
>          // Initialize the copier.  The 0x8000 should match the size of the
>          // buffer our buffered stream is using, for best performance.  If we're
>          // not using our own buffered stream, that's ok too.  But maybe we
>          // should just use the default net segment size here?
> +        copier.initBehavior(true, 0x8000);

Nit: probably should include /*aPickSinkForBufferization*/ here, ala the previous code.

Comment 8

[Vladan Djeric (:vladan)]

Comment on attachment 820270 [details] [diff] [review]
Possible nsIAsyncStreamCopier API

Review of attachment 820270 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIAsyncStreamCopier.idl
@@ +62,5 @@
> +     * Enum constants for argument aBuffering
> +     */
> +    const long BUFFERING_BUFFERED = 0;
> +    // Stream is known to be buffered, i.e. ReadSegments is implemented
> +    const long BUFFERING_UNBUFFERED = 1;

BUFFERING_UNBUFFERED is a bit of an odd name
How about BUFFERED_STREAM, UNBUFFERED_STREAM, AUTODETECT_BUFFERING?

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 820270 [details] [diff] [review]
Possible nsIAsyncStreamCopier API

Review of attachment 820270 [details] [diff] [review]:
-----------------------------------------------------------------

few questions:

- what does mean for you 'opening URI on a non-main thread'?  AFAIK, most URL handlers are designed only to work on the main thread ; only for http loads (via nsHttpChannel) you can (in OnStartRequest) redirect OnDataAvailable calls to a different thread, but still you have to AsyncOpen on the main thread

- what happens when you specify BUFFERING_BUFFERED on both initSink* and initSource* calls?  It doesn't make sense, the logic has to either fail because the args were given wrong or pick something itself ; I'd rather specify the buffering through initBehavior as one of: SINK_BUFFERED, SOURCE_BUFFERED, AUTODETECT 
  - when both sink and source are found unbuffered, buffer the source
  - when both found buffered, prefer source buffering (based on performance reasons)

Comment 10

[David Teller [:Yoric] - still alive but not very active]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> Comment on attachment 820270 [details] [diff] [review]
> Possible nsIAsyncStreamCopier API
> 
> Review of attachment 820270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> few questions:
> 
> - what does mean for you 'opening URI on a non-main thread'?  AFAIK, most
> URL handlers are designed only to work on the main thread ; only for http
> loads (via nsHttpChannel) you can (in OnStartRequest) redirect
> OnDataAvailable calls to a different thread, but still you have to AsyncOpen
> on the main thread

Mmmh... That's not good. The problem I wish to solve here is to ensure that we can open streams without blocking the main thread. Some streams don't do any I/O during the call to Open/AsyncOpen (i.e. File streams with the correct flags), but some do (i.e. Jar streams). It was my hope that opening streams directly from URIs out of the main thread would solve everything in one swoop, but if these URL handlers are not thread safe, this means I'll have to go down in the trenches.

What would you suggest?


> - what happens when you specify BUFFERING_BUFFERED on both initSink* and
> initSource* calls?  It doesn't make sense, the logic has to either fail
> because the args were given wrong or pick something itself ; I'd rather
> specify the buffering through initBehavior as one of: SINK_BUFFERED,
> SOURCE_BUFFERED, AUTODETECT 
>   - when both sink and source are found unbuffered, buffer the source
>   - when both found buffered, prefer source buffering (based on performance
> reasons)

As you saw in the other bug, these (possibly misnamed) flags are meant to decide what to do if neither buffer is buffered yet.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #10)
> (In reply to Honza Bambas (:mayhemer) from comment #9)
> > Comment on attachment 820270 [details] [diff] [review]
> > Possible nsIAsyncStreamCopier API
> > 
> > Review of attachment 820270 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > few questions:
> > 
> > - what does mean for you 'opening URI on a non-main thread'?  AFAIK, most
> > URL handlers are designed only to work on the main thread ; only for http
> > loads (via nsHttpChannel) you can (in OnStartRequest) redirect
> > OnDataAvailable calls to a different thread, but still you have to AsyncOpen
> > on the main thread
> 
> Mmmh... That's not good. The problem I wish to solve here is to ensure that
> we can open streams without blocking the main thread. Some streams don't do
> any I/O during the call to Open/AsyncOpen (i.e. File streams with the
> correct flags), but some do (i.e. Jar streams). It was my hope that opening
> streams directly from URIs out of the main thread would solve everything in
> one swoop, but if these URL handlers are not thread safe, this means I'll
> have to go down in the trenches.
> 
> What would you suggest?

First I don't think you need to do anything here.  If you believe that using channels to fetch URIs can help you then just use channels as others do - create, AsyncOpen and deliver data on the main thread.  Most implementations (and I think JARs too?.. not sure) should be implemented in a way that no main thread I/O is involved.  I can only speak for http and somewhat of ftp and wyciwyg.  

In case of http there is no I/O according access to _data_ (at least with the new HTTP cache that is currently in the tree, half finished and for now disabled).  However, e.g. adding cookies and http auth may involve some main thread I/O.  All these are real tracked bugs.

What I want to say is that you are OK to use channels on the main thread.  When there is an I/O on the main thread caused by using channels, then you shouldn't try fixing them by moving open/read of URI channels to background in the stream copier, but file them (or check whether not already filed) as regular bugs that should be fixed in their respective components.


Also, you are talking about 'streams'.  Having a stream is something different than having a channel.  Channel is using to async fetch data behind a URL to generally main thread consumers.  Stream is something that has already been open or at least setup to deliver data to anyone that reads it.  There is a variety of stream in the platform.  But when you are fetching a URL, you only have a channel and OnStart/OnData/OnStop notifications.  Not a stream.

> 
> 
> > - what happens when you specify BUFFERING_BUFFERED on both initSink* and
> > initSource* calls?  It doesn't make sense, the logic has to either fail
> > because the args were given wrong or pick something itself ; I'd rather
> > specify the buffering through initBehavior as one of: SINK_BUFFERED,
> > SOURCE_BUFFERED, AUTODETECT 
> >   - when both sink and source are found unbuffered, buffer the source
> >   - when both found buffered, prefer source buffering (based on performance
> > reasons)
> 
> As you saw in the other bug, these (possibly misnamed) flags are meant to
> decide what to do if neither buffer is buffered yet.

You have not answerd the question:

- what happens when you specify BUFFERING_BUFFERED on both initSink* and
initSource* calls?

It seems like BUFFERING_BUFFERED indicates that you know the stream *IS* buffered and there should be no automatic decision algo involved.

However, this is not that critically important.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

> You have not answerd the question:
> 
> - what happens when you specify BUFFERING_BUFFERED on both initSink* and
initSource* calls?
> 
> It seems like BUFFERING_BUFFERED indicates that you know the stream *IS* buffered and there should be no automatic decision algo involved.

Actually, I was attempting to keep the original semantics of nsIAsyncStreamCopier. I personally don't like it enormously, but I assumed that there was some reason to design it as it was.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Well, looks like this is a WONTFIX. We'll need to handle each channel kind separately.