support partial reads from OnDataAvailable events

trying for 0.9.6

-> 0.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 converter). so, i've thought about two solutions: 1) have each stream listener that supports chaining implement the following interface: 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 :)

see bug 99638 for an example of how dynamic aggregation would also help.

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

punt -> 0.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. eg: NS_IMETHODIMP MyListener::OnStartRequest(nsIRequest *req, nsISupports *ctx) { nsCOMPtr<nsIRequest> baseReq; req->GetBaseRequest(getter_AddRefs(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 channel. 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.

-> need any interface changes to be resolved by 1.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 :-) thoughts? 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 converters).

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 class. 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

mass futuring of untargeted bugs

no, this does not in any way block 1.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.

is this a "meta" bug?

no, although what's left here might be the same as bug 227113... not sure