62566 - nsIChannel::AsyncWrite interface needs to be revised

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Darin Fisher]

I've been thinking for some time now about changing the interface
of nsIChannel::AsyncWrite.  The way it is currently defined doesn't
seem to be very consistent with AsyncRead, and my feeling is that
now is the right time to fix it.

I've been discussing this some on the newsgroup and with others in
the necko group.  There's been some interesting feedback.  Basically,
my change to AsyncWrite is actually more or less a cosmetic change...
it is just a matter of clarifying what AsyncWrite really means.

What I want to do is change AsyncWrite from:

  AsyncWrite(in nsIInputStream source,
             in nsIStreamObserver observer,
             in nsISupports ctxt);

to

  AsyncWrite(in nsIStreamProvider provider,
             in nsISupports ctxt);

where nsIStreamProvider is a new interface defined as:

  interface nsIStreamProvider : nsIStreamObserver
  {
    void onProvideData(in nsIChannel channel,
                       in nsISupports ctxt,
                       in nsIOutputStream output,
                       in unsigned long offset,
                       inout unsigned long count);
  };

The offset parameter is informational only and, in most cases, it
can simply be ignored.  It tells you what the current stream position
is and hence how much has been written.  The count parameter tells
you how much data you can write to the output stream w/o blocking.
On return from onProvideData, the count parameter should indicate
the number of bytes actually written to the output stream.

The point is to make it very clear that the channel will request
data from the consumer when it knows that the underlying transport
is ready to accept it.  If you think about it, this is no different
than having the channel call Read on the source input stream... it
just clarifies what we are really doing (IMO).

EOF would be indicated by returning NS_BASE_STREAM_CLOSE from the
consumer's onProvideData implementation.  If data were temporarily
unavailable, the consumer's onProvideData could suspend the AsyncWrite
by returning NS_BASE_STREAM_WOULD_BLOCK.  It would be up to the
consumer then to call Resume on the channel to continue the AsyncWrite
once there was more data for writing.

I'm attaching a preliminary nsIStreamProvider IDL file for review.

Comment 1

[Darin Fisher]

Attachment: nsIStreamProvider.idl

Comment 2

[Darin Fisher]

One of the biggest problems with this solution (I'm just realizing) is that
of trying to proxy the onProvideData callback from one thread to another as
would be the case for the socket transport implementation.  I'm not sure how
to convey onProvideData's return values to the caller if onProvideData is
proxied using an event queue.  Any ideas?

Comment 3

[Doug Turner (:dougt)]

Darin, all looks good.  however, what happens if I have an existing 
nsIInputStream?  

Comment 4

[Darin Fisher]

Actually, this is not a problem.  In the case of a proxied onProvideData.
The correct return value would be NS_BASE_STREAM_WOULD_BLOCK with zero
bytes written.  Later on, when the provide data event is handled, the
consumer would possibly write some data, and then the proxy implementation
could resume the channel as appropriate.

This would work just fine for the socket transport, in which the output
stream passed to the provider would be the write end of a pipe.

This issue also clarifies one important point:  the output stream passed
to the onProvideData callback can be written to at any time... NOT just
from within the implementation of onProvideData.

Comment 5

[Darin Fisher]

In the common case that you have an input stream to your data, a utility class
could be provided that implements nsIStreamProvider for you.  That utility
class' onProvideData implementation would simply call output->WriteFrom(input).

I imagine a C++ helper function of the form:

  NS_AsyncWriteFromStream(channel, inputStream);

and even:

  NS_AsyncReadToStream(channel, outputStream);

which would allow a consumer to easily pass all data read from a channel
to an output stream.

Comment 6

[Darin Fisher]

I have a patch and some additional files which makes this change to AsyncWrite.

There are a number of other changes that are included in this patch.  They may
not appear to be directly related to AsyncWrite, but rather they are about
making both AsyncWrite and AsyncRead consistent with one another and complete.

The changes include:

1. Patched nsIChannel.idl with the new AsyncWrite prototype.

2. Added nsIStreamProvider.idl which defines the following interfaces:

   nsIStreamProvider       - OnProvideData method
   nsIStreamProviderProxy  - Proxies nsIStreamProvider calls between threads.
   nsISimpleStreamProvider - Provides data from an nsIInputStream, simulating
                             the old AsyncWrite interface.
                             See NS_AsyncWriteFromStream()

3. Modified nsIStreamListener as such:

   nsIStreamListener       - Cleanup meaning of OnDataAvailable return codes.
   nsIStreamListenerProxy  - Replaces nsIAsyncStreamListener
   nsISimpleStreamListener - Writes data to an nsIOutputStream in response to
                             OnDataAvailable.  See NS_AsyncReadToStream().
   moved CID's to nsNetCID.h and factory methods to nsNetModule.cpp

4. Modified nsIStreamObserver as such:

   nsIStreamObserverProxy - Replaces nsIAsyncStreamObserver
   moved CID's to nsNetCID.h and factory methods to nsNetModule.cpp

5. Added implementations for:

   nsIStreamObserverProxy
   nsIStreamListenerProxy - incorporates a pipe
   nsIStreamProviderProxy - incorporates a pipe
   nsISimpleStreamListener
   nsISimpleStreamProvider

6. Cleaned up nsFileTransport and nsSocketTransport

   Removed the pipe, as it is now inside the nsIStream{Listener,Provider}Proxy
   implementations.  Results in much simpler transport code.

7. Added NS_ helper functions to nsNetUtil.h

   NS_AsyncWriteFromStream()
   NS_AsyncReadToStream()
   NS_NewStreamObserverProxy()
   NS_NewStreamListenerProxy()
   NS_NewStreamProviderProxy()

8. Patched code throughout the mozilla tree for the new AsyncWrite signature.

   Replaced:  channel->AsyncWrite(stream, observer, context);
       with:  NS_AsyncWriteFromStream(channel, stream, observer, context);

9. Updated test files:

   netwerk/test/TestFileTransport
   netwerk/test/TestSocketIO


I believe that I have tested these changes thoroughly.  Everything seems to
be in good order.  But, some more eyes would of course be nice ;)

Comment 7

[Darin Fisher]

Attachment: Patch and additional files...

Comment 8

[Darin Fisher]

Forgot to set the mime-type on that patch... the file is a .tar.gz containing
the new files relative to the top-level source directory and a diff file, named
patch.

Comment 9

[Darin Fisher]

Created a branch for these changes, tag=DARIN_AsyncWrite_20001227_BRANCH.  This
should make evaluating and revising the AsyncWrite changes a whole lot easier :)

Comment 10

[Darin Fisher]

Attachment: Final revision (new files and diffs from devel branch)

Comment 11

[Darin Fisher]

I've attached the final revision of these changes.  The attachment is a
gzip compressed tar file containing the following 3 files:

newfiles.tgz
netwerk.diff
other.diff

Given todays CVS tip, I found the following commands to work well on a RH 6.2
Linux box:

% cd /path/to/mozilla
% tar xfz /path/to/newfiles.tgz   # adds files to netwerk/
% cd ..
% POSIXLY_CORRECT=1 patch -p0 < /path/to/other.diff
% POSIXLY_CORRECT=1 patch -p0 < /path/to/netwerk.diff

Comment 12

[Chris Waterson]

darin, I took a look at the changes; I think mscott or rpotts would be better
qualified to super-review from a design standpoint than I am. Here are my
nitpicky detail-level comments...

- I notice that you're using warren's new log stuff. I was under the impression
that this had fallen into ill favor. In fact, I thought dbaron was going to rip
it out altogether. brendan, do you have any opinions on this? (It's still in
it's evil `too-few-parens' state...) IMO, we should leave the perfectly good
PR_LOG stuff alone here. Or, if this is worth changing, it should be broken out
into a patch that is separate from what are pretty sweeping architectural changes.

- Does nsFileTransport::Run() really need a monitor? (Or does it just need a
lock?) Does Run() re-enter itself recursively?

- Why didn't you just remove nsWriteToFile from nsFileTransport.cpp? (CVS will
always remember it.) At a minimum, #ifdef it (instead of /* */ commenting it) if
we still need the code for some reason...

- "nsStreamIOChannel::AsyncWrite NOT IMPLEMENTED!", but it looks implemented to
me...what's left to do?

- I'm not a big fan of your montulli-style macros; e.g., GET_INPUT_STREAM() to
wrap your downcasting an interface pointer to an implementation. Zoiks! Why do
you even need to do that? This smacks of some deeper evil to me...are we missing
an interface? Or do we have too many interfaces? Or has the code been factored
wrong? This warrants further investigation IMO.

- There is a bunch of whitespace cleanup (noble! get rid of that goofy style
that ruslan inflicted on the code), but it adds to the noise. You can get rid of
it by adding `-w' to the list of switches you provide to `cvs diff'.

- #if 0'd code in nsAsyncEvent, nsFTPChannel, nsHTTPResponseListener, commented
out mFromStream in nsResChannel. Should we worry about these?

Summary: From the looks of it, you whacked necko pretty hard, and I'm certainly
not familiar enough with the necko guts to comment on that. I'd recommend that
we sort of the nsIStreamProvider changes from the whitespace cleanup and warren
PRINTF() foo, and then go another round on this with rpotts or mscott at the wheel.

Comment 13

[Darin Fisher]

Chris, thanks for the comments.


PRINTF()
 - I kind of like the way nslog.h works, but then if it is going away... ack.
   So, I will revert to PR_LOG for now.

nsFileTransport::Run()
 - What it needs is a re-entrant lock... because Cancel calls Resume.  Though
   Cancel could be modified to avoid this.

nsWriteToFile
 - Agreed... should be removed instead of just commented out.

nsStreamIOChannel::AsyncWrite NOT IMPLEMENTED!
 - Ooops... I somehow overlooked this when re-implementing the function.

montulli-style macros
 - Wanted the benefits of nsCOMPtr for member variables.  But, it sounds like
   I'm abusing things... will change.

whitespace cleanup
 - So the whitespace cleanup is good, but for review purpose -w is better.
   No problem.

nsAsyncEvent/nsFTPChannel
 - No worries here... dougt has a new FTP implementation that will override all.

nsHTTPResponseListener
 - I will delete the #if 0'd section.  It is no longer needed.

mFromStream in nsResChannel
 - This member variable is no longer needed and should have been deleted.

Comment 14

[Darin Fisher]

Attachment: Revised patch per waterson's comments (.tgz file)

Comment 15

[Darin Fisher]

**spam** ignore that last attachment.

Comment 16

[Brendan Eich [:brendan]]

This is an uber-nit, forgive me: the onProvideData name bugs me.  Why isn't it
onDataProvided, a la onDataAvailable?  Oh, it's following the onStartRequest and
onStopRequest pattern.  But the English is fractured, and this is about "data",
not about a "request", so I think the nsIStreamListener::onDataAvailable
precedent wins.

If the above is not persuasive, ignore me -- it's your choice.

/be

Comment 17

[Darin Fisher]

I agree that the English is fractured, but OnDataProvided has a different
meaning than OnProvideData.  What I am after is a name which reflects the
fact that the channel is ready for its client to provide it with some data,
and OnDataProvided is not that.

Suggestions???  I've never been completely sold on calling it OnProvideData...
I just haven't encountered anything better.

Comment 18

[Darin Fisher]

Perhaps OnDataWritable would be better?

Comment 19

[Brendan Eich [:brendan]]

onDataWritable works for me!
 Now I just have to pretend that onDataAvailable is onDataReadable.

/be

Comment 20

[Darin Fisher]

That's really the only reason I have shy'd away from OnDataWritable b/c
then I would want... really want... to have OnDataReadable too.  But,
the more I think about it, perhaps this isn't such a big deal.  OnDataWritable
is a whole lot better than OnProvideData... we can always make the switch
to OnDataReadable later on down the road.

So, with that said I think I will make the change to OnDataWritable.

Comment 21

[Darin Fisher]

Attachment: Revised patch per waterson's comments #2 (.tgz)

Comment 22

[Darin Fisher]

I've attached another revision of my patch that incorporates changes following
waterson's comments as well as the switch from OnProvideData to OnDataWritable.
The diff files were generated with the -w flag, so don't be surprised if the
indentation is wacked.  The attachment is a gzip compressed tar file containing
the following 3 files:

newfiles.tgz
netwerk.diff
other.diff

I used the following commands (on a rh7 linux box) to apply these changes:

% cd /path/to/mozilla
% cd ..
% tar xfz /path/to/newfiles.tgz   # adds files to netwerk/
% POSIXLY_CORRECT=1 patch -p0 < /path/to/other.diff
% POSIXLY_CORRECT=1 patch -p0 < /path/to/netwerk.diff

Comment 23

[Darin Fisher]

Attachment: Yet another revision: removed a lock from nsStreamListenerProxy

Comment 24

[Darin Fisher]

Attachment: FTP patch (dougt recently checked in a bunch of FTP changes)

Comment 25

[Darin Fisher]

One thing that is not clearly defined in this bug report is the motivation for
these changes.  I want to clarify those here:

1) The change to the AsyncWrite interface will make it possible to initiate
   an AsyncWrite when only part (or none) of the data to be written is
   available.
   ** This will be an essential requirement of the new cache design.
   ** This is required by FTP to avoid busy waiting on the control connection
      (see bug 65220).

2) The implementation of nsStreamListenerProxy (replacing nsAsyncStreamListener)
   enhances performance by coalescing OnDataAvailable events when possible.
   My initial testing shows that this can reduce the number of OnDataAvailable
   events posted to the UI thread by a factor of 2 (on average) from the socket
   transport thread.  There is effectively no gain for proxying from a file
   transport thread since each read usually fills the pipe completely.

3) Suspend and resume are basically non-functional in the current code base,
   and this patch is a first step toward providing such functionality.

Comment 26

[Scott MacGregor]

I'll look at these patches today and thanks for the explanation as to why we
want to make these changes.

Comment 27

[Scott MacGregor]

Hey Darrin, a couple of comments / questions:

1) you list a copule factory methods in netwerk as being deprecated
(NS_NewSyncStreamListener, NS_NewAsycStreamListener, etc.) Can those be removed
or do we still have callers using these deprecated methods? I'm always worried
when I see methods declared as deprecated. Do we need another bug to clean up
the callers?

2) do you have a mac person to help land mac project changes for these new
files? (or do you have access to a make where you've already made these changes.

3) for kicks, can you try reading some imap messages. Sending some mail and also
reading a local mail message (which uses the file transport to read byte
ranges). With local mail, try reading multiple messages in a file to make sure
the byte range stuff still works on the file transport. 

4) can mSourceWrapper in nsFileTransport be a nsCOMPtr instead of a raw ptr?

5) Are we sure all this runs on all 3 platforms. Just nervous because of the
size of the changes and the fact that it's happening to a core piece of
infrastructure (netlib)

6) I noticed we had an XP_WIN16 #defines in nsSocketTransport.cpp. Wow, I wonder
if we can remove those? (this comment is of course unrelated to your changes)

7) can mSocketOutputStream/mSocketInputStream in nsSocketTransport be  nsCOMPtrs?

8) I'm glad to see that necko is going to get a CID file! =) Hopefully we can
put more class ids in there instead of in the idl files. I'm a big fan of using
a CID file for modules.

9) I missed something. nsStreamListenerProxy didn't always use a pipe did it? Is
this new with your new implementation files? 

Most of these comments are small or just me asking questions about things I
didn't understand. 

Overall everything looked really good. I liked how well the code was written.
This is a big landing which is always fun =). Some of my questions are just
making sure we have our bases covered with regards to building on all platforms
and adding some extra tests to make sure things still work.

sr=mscott 

Comment 28

[Darin Fisher]

Scott, thanks for the comments.

1) Removing the depracated calls should be a new bug.  Otherwise, my changes
   would touch even more files ;)

2) No mac person to help me.. sigh!  But, I do have a mac build that I have
   been using for testing.

3) I will spend some time exercising Imap (in the ways you suggest) before
   landing these changes.

4) See Waterson's comments... mSourceWrapper was originally a nsCOMPtr, but
   I was always having to cast it to the raw pointer in order to use it.  The
   code actually became cleaner if I just used a raw pointer to begin with.
   (It would be nice if we had a nsCOMPtr-type class that worked for non
   interface pointers!)

5) I'll be careful ;)

6) I didn't realize XP_WIN16 was dead.

7) See point 4 above.

8) Yes!!  I am trying to gradually convert necko to using CID files.

9) One of my changes is to add the pipe to nsStreamListenerProxy.  In this way
   both the file transport and the socket transport can reuse the code that
   cares about the pipe.  Moreover, in my opinion a stream listener proxy would
   always need a pipe to provide thread safety... so it just naturally belongs
   in the proxy itself.

Comment 29

[Darin Fisher]

These changes have now landed on the trunk :-)

Comment 30

[Chris Petersen]

*** Bug 107902 has been marked as a duplicate of this bug. ***