65272 - nsIChannel does not support overlapped io

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

nsIChannel - transferCount & transferOffset can't apply to both read and write
.

After the reduction of parametes to AsyncRead and AsyncWrite, we have broken the
ablity to set different transfer counts and transfer offset.  Suppose that you
wanted to read at w for x bytes, and needed to write at y to z.  With the
current nsIChannel interface, you would have a tough time doing this.  

AsyncRead and AsyncWrite should have their count's and offset's explictly set in
the method.

Comment 1

[Darin Fisher]

But, with most channels you cannot read and write simultaneously.  This is
definitely true of the file transport, and unless there are cases in which
we really need to support overlapped reading and writing, we should probably
just prohibit it flat out.  Does FTP require overlapped reading and writing
to sockets?

Comment 2

[Doug Turner (:dougt)]

There is a difference between what the implementation can do and what the
interface describes.  nsIChannel should be able to describe an inplemention that
can do "overlapped reading and writing".  If I wanted only to read X bytes and
read y bytes async, I would be out of luck.  This means that the nsIChannel
interface does not describe something it should.

Comment 3

[Darin Fisher]

so long as X < y, then you could always Cancel the operation with a success
code... if X > y, then you are out of luck because there is no more data.

but anyways, it sounds like you are saying that overlapped reading and writing
is desired... so then i totally agree... there is no other solution than to add
the parameters to AsyncRead and AsyncWrite.  i wonder why they were ever removed
in the first place.

Comment 4

[Darin Fisher]

I must take back one of my comments:  For the socket transport, it is not
possible to Cancel with a success code.  Instead, you would have to set
mBytesExpected to zero, but there is no guarantee that there will not be
other OnDataAvailable events in the queue.  So, effectively, there is no
way to successfully stop an AsyncRead (ie. stop reading but stay connected).

Comment 5

[Doug Turner (:dougt)]

Currently there is no way to 

(a) suspend, 
(b) resume, 
(c) cancel, 
(d) get status of, 
(e) set transferCount 
(f) set transferOffset 

(possibly more) 

of seperate AsyncRead()/AsyncWrite calls.  For example, if you open a socket 
transport and want to both read and write to this socket, there is no way to 
cancel just the 'write' part.  

So, how do we support overlapped io?

1.  Add a flag field to most of the calls in nsIRequest and some of the calls in 
nsIChannel so that we can specify which operation (read|write) we are talking 
about.

Problem - massive changes and some nsIRequest implementions do not think in 
terms of read|write.

2.  make the socket transport service's CreateTransport() return two channels, 
one for the read part and one for the write part.

Problem - No other nsIChannel will benefit from this cheap fix.  We will have a 
broken story when it comes down to overlapped IO.

3.  Make nsIChannel AsyncRead and AsyncWrite return a nsIRequest.  State will be 
return in this nsIRequest so that all channels can produce both a read and write 
 request.  Implemention can be in phases so that only the STS needs to return a 
"real" request (other implementers can return nsnull or itself)


I am leaning toward #3.  I think that it is the cleanest. 

Comment 6

[Darin Fisher]

I like #3 too, but would nsIChannel still derive from nsIRequest?

Comment 7

[Judson Valeski]

Note: overlapping I/O does work in nsIChannel (specifically on the socket 
transport), in the *synchronous* case (GetInput|OutputStream), *not* the async 
case which is what we're discussing here.

I like option #3 as well, but it confuses the fact that current channel's impl 
both nsIChannel and nsIRequest. The current nsIRequest's meaning becomes 
ambiguous if I also get one back from Async*().

Comment 8

[Darin Fisher]

Also, we would probably want to pass the nsIRequest value to all of the
nsIStreamListener methods, right?

void OnDataAvailable(in nsIChannel channel,
                     in nsIRequest request,
                     in nsISupports context,
                     in nsIInputStream stream,
                     in unsigned long offset,
                     in unsigned long count);

Comment 9

[Darin Fisher]

A channel's implementation of nsIRequest could simply apply to all channel
operations, so that you could _completely_ Suspend a channel, for example.
Implementations of Suspend and Resume would have to use a suspend counter
(a la nsSocketTransport.cpp) to make this work.

Comment 10

[Doug Turner (:dougt)]

as for OnDataAvailable, I would hate to add yet another parameter.  How about 
adding an attribute on nsIChannel which can return the nsIRequest for either a 
read|write?


Jud, I understand your concern about returning an nsIRequest.  I would think 
that eventually, after all nsIChannel implementions are refactored so that the 
state information is part of this return nsIRequest, the nsIChannel would not 
derive from a nsIRequest.  Maybe, we should just do this now and make the 
actually implementations explictly define themselves as both nsIChannels and 
nsIRequests.

Comment 11

[Darin Fisher]

After some thought, doing away with nsIChannel deriving from nsIRequest is
really good, because it removes concepts such as Suspend/Resume/Cancel
from synchronous channel i/o.  It makes good sense to say that only async
operations can be suspended and canceled.  Synchronous operations do not
need explicit calls for these... nsI{In,Out}putStream::Close() work just fine.

As far as adding the request to nsIStreamListener methods, I like it because it
allows the listener to be able to identify the request corresponding to the
AsyncRead.  Of course, this could be a parameter on the channel, but then in
the world of proxied events, you might get into trouble.  OnStopRequest could
be fired while OnDataAvailable events were yet to be processed.  After calling
OnStopRequest, a channel would want to RELEASE the request object.  This would
mean that for the OnDataAvailable events not yet processed, there would be
no associated request object!  Oops!

Because of this I would argue that it is bad to allow access to these requests
via nsIChannel.  Otherwise, you would have difficulty determining when it is
okay to call AsyncRead a second time on a channel.  I would prefer to have
nsIRequest visible only as a return value of Async{Read,Write} and as arguments
to OnDataAvailable, etc.

BTW, if we "protect" the nsIRequest in this way, it would be possible to have
the stream listener proxy implementation define its own nsIRequest
implementation as a wrapper for the channels request object.  Thereby giving
the proxy the ability to know when the operation has been suspended/resumed.
Doug: remember we were talking about how to plug stream listener proxies onto
any arbitrary channel from the callsite of AsyncRead... well this would be
enough to allow for that!

Comment 12

[Doug Turner (:dougt)]

Cut a branch with the start of these changes. 

Branch Tag:  DOUGT_CHANNEL_CHANGES_01222001_BRANCH

Comment 13

[Doug Turner (:dougt)]

This bug should be fixed my recently checked in necko changes.  

Comment 14

[Tom Everingham]

verified