Closed Bug 93055 Opened 23 years ago Closed 9 years ago

Fully support suspending nsIStreamListener events


(Core :: Networking, defect, P3)






(Reporter: darin.moz, Unassigned)


(Blocks 1 open bug)


(Keywords: arch, perf)

support partial reads from OnDataAvailable events
Blocks: 92477
Target Milestone: --- → mozilla1.0
Blocks: 95314
Blocks: 101711
Blocks: 7251, 71668
trying for 0.9.6
Target Milestone: mozilla1.0 → mozilla0.9.6
Priority: -- → P2
-> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
so, let me outline the problem of "supporting partial reads" a bit:

consider a chain of stream listeners connected to a channel:

  channel -> listener -> ... -> listener

we are interested in providing the following API to each listener:

  to suspend the stream of data, either
  1) return NS_BASE_STREAM_WOULD_BLOCK from your OnDataAvailable
  2) or, call Suspend() on the nsIRequest passed to your OnDataAvailable

  to resume the stream of data:
  call Resume() on the nsIRequest passed to your OnDataAvailable

when you furthermore consider that listeners in the chain could be converting
the stream from one format to another, the problem takes shape. each listener
must be able to buffer data if necessary, and this buffering must play nicely
with the suspend/resume API.
the nsIRequest interface also provides an isPending boolean attribute, which
must be TRUE if there is still more data coming down the pipe, and FALSE
otherwise.  note: it's possible for the channel to be "done" sending data,
but for there to still be data in the pipe (eg. data buffered by a stream
so, i've thought about two solutions:

1) have each stream listener that supports chaining implement the following
   interface nsIChainedStreamListener : nsISupports
      boolean hasBufferedData();
      void flush();
if each "chained" listener implemented this interface, then it would be possible
for the channel (which implements nsIRequest) to correctly implement isPending
(by calling hasBufferedData down the chain) and resume (by calling flush down
the chain).  a "chained" listener would call the onDataAvailable of it's
listener in response to flush if it had buffered data.

this solution is ok, but perhaps not great.

2) each "buffering" stream listener in the chain could pass its own nsIRequest
implementation to its listener instead of passing the channel.  (note: the
multipart stream converter does this already for other reasons).  the
"buffering" stream listener would then be able to intercept calls to resume and
isPending directly.  we wouldn't rely on chaining calls to flush and
hasBufferedData (ie. there would be little modification to the channel code).

the problem with this solution is that a generic "buffering" stream listener
cannot possibly satisfy QueryInterface for the underlying channel.  and, since
much of our code relies upon accessing the underlying channel by QI'ing the
nsIRequest passed to OnDataAvailable, this is a major problem.

the first thing that popped into my mind was: hmm.. perhaps this is an
application for some form of dynamic aggregation?!?  the "buffering" stream
listener's nsIRequest implementation would be the aggregator and the channel
would then become the aggregatee.  BUT, in COM, aggregation is not dynamic,
meaning that it must be established at the point of creation of the aggregatee.
this won't work for channel and stream converters... we need to dynamically 
create the "outer" nsIRequest well after when the channel is created.
perhaps some hybrid "dynamic" aggregation model could be employed?!?

either way, this second approach seems like it would also better solve the
multipart stream converters problems as well (eg. you currently can't get to the
underlying channel of a multipart stream converter if all you have is a
reference to its dummy channel "nsPartChannel").
so, i'm considering an interface like this:

   interface nsISupportsAggregation : nsISupports
     attribute nsISupports unknownOuter;

where |unknownOuter| could be NULL.  i suspect i'm totally overlooking some
crucial COM axiom that will make this impossible, but perhaps not :)

anyways, the implementation of nsISupportsAggregation would do the nsAgg.h
stuff to support aggregation, but would dynamically support switching between
using fOuter and fAggregated.

suggestions very much welcome :)
Keywords: arch, perf
see bug 99638 for an example of how dynamic aggregation would also help.
No longer blocks: 92477
interface nsIRequest {

  nsIRequest baseRequest;

maybe a better solution.  a channel would return itself as the baseRequest.  a
stream converter on the other hand would return the channel as the baseRequest.

-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
punt -> 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
even if full support for partial reads is not implemented by mozilla 1.0, i
think the required interface changes must be put into place.  i believe that
nsIRequest::baseRequest is all that should be necessary.

this would mean:

1) consumers of a channel piped through a stream converter would have to get the
base request from the nsIRequest passed to it via OnStartRequest,
OnDataAvailable, and OnStopRequest in order to query the underlying channel.


  MyListener::OnStartRequest(nsIRequest *req, nsISupports *ctx)
     nsCOMPtr<nsIRequest> baseReq;
     NS_ASSERTION(baseReq, "nsIRequest::baseRequest must be non-null");
     nsCOMPtr<nsIChannel> chan( do_QueryInterface(baseReq) );
     if (chan) {
       // ... do stuff with the channel

2) stream converters would all need to be modified to support partial reads. 
this most likely means creating a base class for stream converters that knows
how to do partial reads on behalf of the _real_ stream converters.  this
shouldn't be too much work.

3) channels would most likely want to interalize any stream converters they use.
 for example, http adds stream converters for gzip decompression.  the http
channel should intercept the nsIStreamListener notifications from the stream
converter so it can pass itself as the nsIRequest.  this would allow consumers
to avoid the baseRequest hoopla when they know they are talking directly to a

4) many consumers would have to be changed to call GetBaseRequest since most
nsIStreamListener's may be upstream from the unknown decoder (which is a stream
converter that does buffering).

so QUESTION: is it wrong to add this extra layer of indirection to the API? 
will it just cause tons of regressions and pain for existing consumers (mozilla
programmers)?  is there a viable alternative?

feedback would be most appreciated :-)
I worry about overengineering a solution. I think its too much work for a
solution which:

a) Won't be testable at the time its implemented, and so the desireable
behaviour will be both frozen and never tested. Design discussions are all very
well, but you often need to change things afterwards.

b) Involves getting extra data members all the time, lossing out on extra COM stuff.

c) Is not transparent. Consumers should simply be able to read as much as .they
want, and the _stream_ should deal with it by keeping track internally

Each object in the chain should have a buffer of size x available (like it
currently does). If their consumer doens't need it all, they can suspend() their
upstream source. When resuming, they can call resume upstream.

(Yes, this has problems, but I think the approach is workable)

Now, this (or any other solution) is unable to be finished by 1.0. So the
question is: "What are we freezing?"  nsIInputStream doesn't even have an
UNDER_REVIEW tag to it, and I don't think there are any frozen necko interfaces
- nsIURI isn't frozen yet, even.. 

OTOH, nsIInputStream is in XPCOM, not necko, so it may be more likely to be
frozen than nsIRequest. According to the api review doc, nsIRequest requires
nsILoadGroup, which isn't frozen either, and has its own TODO items. Are we
really going to get to all of these in the next couple of months? Do we really
want to specify this behaviour now?

If we do, then maybe just exposing the base member as proposed would be OK, and
we could have a wrapper class which does the suspending. I'm not convinved that
that would work, though.
Blocks: 124031
-> need any interface changes to be resolved by 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
it turns out that there is another solution to this problem that i hadn't really
thought about before...

if we guarantee that the nsIInputStream delivered in each OnDataAvailable is the
same nsIInputStream pointer on each event, and if we guarantee that the
nsIInputStream can be read at any time after the first OnDataAvailable event,
then we can resolve this bug as fixed.

we'd have to decide what the aOffset and aCount parameters to OnDataAvailable
mean when the stream already has data in it, but aside from that we'd have a
working model for partial reads from ODA events w/o any need to add new
parameters or fixup callers, etc. which is a real big win :-)


moreover, i can push this solution off until post 1.0 because there is no real
API change required.  just some fixup inside necko (especially with the stream
sounds like a great plan.  However, we must ensure that our channels DO only
pass back one stream for the entire OnStart-OnData-OnStop cycle.  The stream
converter and others may be creating temporary string streams, every one
different for every ODA.  This probably can all be solved with a simple wrapper

The other concerning part of this is life cycles of the channel object.  I think
that we may run in to cycles as the channel probably shouldn't go away until the
stream does.  If that is the case, how will the stream let the channel know when
it can go away?  Maybe this isn't an issue... maybe channels just "spawn"
streams and there is no directly relationship.

good point... i need to think about that one.

we probably need to have a cycle between the stream and channel (heck, the
channel might even be the stream).  in that case, we need to know when the
consumer has released their reference to the stream or has otherwise indicated
that they will not be using the stream (e.g., they called
nsIInputStream::close).  once we know that the consumer no longer needs the
stream, we can allow OnStopRequest cleanup to occur.  of course, if
OnStopRequest fires and the stream still has data, we can't completely cleanup
the channel and must defer some cleanup until the stream is empty.

so, maybe we just wait til EOF on the stream (or nsIRequest::cancel) to perform
deferred cleanup.

at any rate, i think we have a solution that'll do the trick w/o requiring any
API modifications (other than documenting that the nsIInputStream will not
change from ODA to ODA).

-> moz 1.1
Priority: P2 → P3
Target Milestone: mozilla1.0 → mozilla1.1
No longer blocks: 124031
Target Milestone: mozilla1.1alpha → ---
mass futuring of untargeted bugs
Target Milestone: --- → Future
Blocks: 69931
Blocks: 227113
Flags: blocking1.8a6?
no, this does not in any way block 1.8a6.
Flags: blocking1.8a6?
(In reply to comment #13)
> mass futuring of untargeted bugs

Might as well just mass "WONTFIX".
OK, this bug as summarized is actually WONTFIX.  nsIStreamListener is frozen,
and we potentially have consumers of this interface both implementing and
calling OnDataAvailable, so we really cannot change its rules.

However, the original problem remains.  Right now, we allow people to Suspend
the nsIRequest, and then as a result they will not (or should not) receive
another OnDataAvailable callback until they call Resume on the nsIRequest.  This
works in most cases except when stream converters are layered on top of
channels.  In comment #3 I mention chains of stream listeners (and in particular
stream converters).  Hence, the original problem exists, but any solution must
keep nsIStreamListener intact.

Updating the summary to better reflect the true intent of this bug.
Summary: support partial reads from OnDataAvailable events → Fully support suspending nsIStreamListener events
Assignee: darin → nobody
QA Contact: benc → networking
is this a "meta" bug?
no, although what's left here might be the same as bug 227113... not sure
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.