11232 - implement helper function for blocking reads

Status: RESOLVED WONTFIX

(Core :: Networking, enhancement, P1)

Description

[Chris Waterson]

Here's a bugper our discussion today. Implement a helper function that opens a
URL and a stream listener, reads the contents of the stream, and then returns
once the read is complete.

Basically, take the functionality of rdf_BlockingParse() and move it into a
necko utility.

Comment 1

[Chris Waterson]

Set self as QA contact.

Comment 2

[Warren Harris]

This may or may not relate to the CopyFrom stuff dougt is doing. Cc'ing him.

Comment 3

[leger]

Spoke with warren at bug triage today.  Not an M10 blocker...moving to M11.

Comment 4

[Warren Harris]

Not essential for m11.

Comment 5

[Warren Harris]

I'm moving this to m15. If we get to it, we get to it, but it doesn't seem like
a big deal.

Comment 6

[leger]

Bulk move of all Necko (to be deleted component) bugs to new Networking

component.

Comment 7

[Warren Harris]

I found 4 places tonight where the same silly helper class was cut&paste in the 
code:

nsXULKeyListener
nsXULDocument
nsRDFXMLDataSource
nsXBLService

You guys should know better than that.

Comment 8

[Warren Harris]

I discovered that we can save some space as well as time here too by 
eliminating this ProxyStream class in favor of an nsIInputStreamObserver that 
notifies BlockingParse when more data is available. It will save us a copy, and 
the copy buffer. Dependent on fixing bug 46777.

Comment 9

[Warren Harris]

I think we should do this now, since I know how to do it and save space and 
time.

Comment 10

[Darin Fisher]

assigning to myself... let me know if this bug is still relavent.  thx!

setting target = future

Comment 11

[Darin Fisher]

waterson: there is a version of NS_OpenURI in nsNetUtil.h that takes a nsIURI
and a nsIStreamListener.  would this not suffice?  is asyncOpen inappropriate
for this?

Comment 12

[Chris Waterson]

In the places where this is used, we actually need to *synchronously* read the 
files in (for example, on startup). Is that the case with the functions you're 
referring to?

Comment 13

[Darin Fisher]

if you are only reading synchronously, then i am confused as to why a stream
listener is involved?  is that you "have time" to block your thread to read from
the stream, but that you still need to pass the data to an asynchronous-type
listener?  if so, then NS_OpenURI wouldn't be the same.

Comment 14

[Chris Waterson]

In the case of RDF, it ``supports'' (*cough*) either blocking or non-blocking
reads. So yeah, I have an async interface that I need to pump synchronously
sometimes.

Comment 15

[Alec Flett]

Brain dump from darin (darin, feel free to correct)

Use the stream transfer service, and create an input transport for the stream.
Using that, create a input stream. this input stream will be a pipe which brings
the data over from the thread that's actually loading the data.

We can't just loop over calls to Available()/OnDataAvailable(), because we can't
call Available() on the pipe. Instead we should call ReadSegments() on the pipe.
In the callback, we create a nsStringInputStream for each chunk, and notify the
stream listener about data being available on that buffer.

My one thought is that is seems like now the consumer (the nsIStreamListener)
will need to call ReadSegments() in order to avoid the extra read out of the
nsStringInputStream, if it wants to avoid the copy out of the pipe. Darin,
thoughts on that?

I've started an initial implementation of the loader service. The one minor
oddity I've run into is that we still have to pass in the nsIChannel for the
stream that's being loaded, so that we have something to pass into
OnDataAvailable() and friends. I guess that isn't a huge deal, but it assumes
there is an nsIChannel available... I guess that's fine for existing
implementations who are already using nsIStreamListener, because they are
getting a channel in anyway.. but I wonder if there are other potential users of
this who have an nsIInputStream created outside of necko.

Comment 16

[Darin Fisher]

> Use the stream transfer service, and create an input transport for the stream.
> Using that, create a input stream. this input stream will be a pipe which 
> brings the data over from the thread that's actually loading the data.

you'll want to open a blocking, buffered input stream on the nsITransport 
returned from nsIStreamTransportService::CreateInputTransport.

  transport->OpenInputStream(nsITransport::OPEN_BLOCKING, ...);

> We can't just loop over calls to Available()/OnDataAvailable(), because we 
> can't call Available() on the pipe. Instead we should call ReadSegments() on 
> the pipe. In the callback, we create a nsStringInputStream for each chunk, and 
> notify the stream listener about data being available on that buffer.

we can allocate the nsStringInputStream once and call the ShareData method to
configure the stream before calling OnDataAvailable each time.

> My one thought is that is seems like now the consumer (the nsIStreamListener)
> will need to call ReadSegments() in order to avoid the extra read out of the
> nsStringInputStream, if it wants to avoid the copy out of the pipe. Darin,
> thoughts on that?

Yes, the consumer should call ReadSegments.

> I've started an initial implementation of the loader service. The one minor
> oddity I've run into is that we still have to pass in the nsIChannel for the
> stream that's being loaded, so that we have something to pass into
> OnDataAvailable() and friends. I guess that isn't a huge deal, but it assumes
> there is an nsIChannel available... I guess that's fine for existing
> implementations who are already using nsIStreamListener, because they are
> getting a channel in anyway.. but I wonder if there are other potential users 
> of this who have an nsIInputStream created outside of necko.

don't think in terms of nsIChannel... think in terms of nsIRequest.  the API
for OnDataAvailable just passes a nsIRequest.  the API for this helper object
should allow the user to configure the nsIRequest and nsISupports for the
OnDataAvailable calls ;-)

darin

Comment 17

[Alec Flett]

ok, I've got a working implementation, over to me. I think I found a tiny bug in
nsPipe3.cpp, and I needed to tweak nsStringStream in order to make it reusable.

Comment 18

[Alec Flett]

Attachment: fix nsPipe3 and nsStringStream

nsStringStream: this is easy - mOffset wasn't being reset when shareData() was
called the 2nd time. This meant that, for instance, if shareData() was first
filled with 4096 bytes, mOffset would be 4096 - when we reused shareData() with
another 999 bytes (for example), then the stream would tell the listener that
it was at offset 4096 bytes even though there were only 999 bytes in the
stream. In the case of the parser, it would try to allocate (length - offset)
which would be 999 - 4096. -3097, which would get translated to some massive
unsigned number making nsMemory::Alloc() fail.

nsPipe3.cpp: The problem I ran into is that when you have a blocking pipe, it
only blocks in ReadSegments when reading the currently available buffers... we
were returning NS_OK even when we hadn't completely filled out the buffers. 

Unfortunately, at some level this seems like the right behavior. In this case
however, we have this stack: (I'm doing this from memory, so bear with me)

nsStringInputStream::ReadSegments()
nsParser::OnDataAvailable()
nsSyncStreamLoader::WritePipeSegments()
nsPipeInputStream::ReadSegments()
nsSyncStreamLoader::PushStreamToListener()

The problem is that when there isn't any more data in the pipe,
nsPipeInputStream::ReadSegments returns early and thus does not completely read
the whole stream, then nsParser::OnDataAvailable() has no way to tell its
caller that it did not completely consume the data... 

So the simple fix was this tweak to nsPipe3.cpp, but I'm wondering if I should
instead be fixing the parser to keep calling ReadSegments() until all the data
is consumed. Help!

Comment 19

[Alec Flett]

Attachment: The nsISyncStreamLoader implementation

here's the implementation... look in the callback, I've put the comments 'How
do we know that everything was consumed?' where I'm confused.

Comment 20

[Alec Flett]

actually, it wouldn't be the parser that would need to be tweaked, the problem
is in the call to 
bufferedStream->ReadSegments(WritePipeSegment, &closure, -1, &bytesRead)

since we don't know how many bytes are actually available, we don't know if the
number of bytes read out of the pipe is all of them or not. (hence the -1)  We
can't call Available() on the pipe because the pipe may or may not know how many
bytes will eventually be available. 

So the problem is basically that I need this:

while (someCondition) {
  bufferedStream->ReadSegments(...);
}

but I don't know how to know "someCondition"


note that my hacked nsPipe3 seems to be working quite well. With that, I've
hooked the loader up to .rdf parsing, among other things, and it really makes
life easy.

Comment 21

[Darin Fisher]

alec: the pipe changes aren't necessary if you sit in a loop waiting for
ReadSegments to return NS_OK and zero bytes read.  that is the EOF indicator.

like this:

  nsresult rv;
  PRUint32 n;
  do {
    rv = stream->ReadSegments(myWriterFunc, this, PRUint32(-1), &n);
  }
  while (NS_SUCCEEDED(rv) && n > 0);

Comment 22

[Darin Fisher]

also, "stream loader" is probably the wrong term for this.  we already have
something in necko called a "stream loader" ... there's the nsIStreamLoader and
the nsIUnicharStreamLoader.  these take a channel and give the user a buffer.

perhaps something like nsISyncInputStreamPump instead??  not great either :(

Comment 23

[Alec Flett]

cool! I'll try that.
 
I'm totally flexible on the name - so nsISyncInputStreamPump works.. let me know
if you have any other names you'd prefer.

Comment 24

[Alec Flett]

oh the pump is stateless too, and I'm using it as a service because of that.

Would there be a use for this within necko? If so, I can make the actual methods
be non-virtual so other consumers can call it directly (rather than going
through the service) and then just use the interface to wrap it... in fact yeah
I'll do that anyway.

Comment 25

[Darin Fisher]

no consumers for this within necko.  also, given that this is a service,
probably good to give it a name ending with "Service" ;-)

nsSyncLoaderService.. hey, now that sounds familiar!  let me ponder the name for
a bit :)

Comment 26

[Alec Flett]

Attachment: NS_NewBlockingInputStream

Ok, darin and I talked about this a bit more.. this is now split into two
things, and here's the first part. 

Here's the split:
1) provide a mechanism similar to nsBufferedInputStream that creates an
intermediary pipe and starts the other input stream on a seperate thread. This
allows you to simply take an nsIInputStream and run it on a seperate thread, no
nsIRequest/etc needed.
2) provide another mechanism to pump an nsIInputStream into a nsIStreamListener
until it is empty. 

Between these two mechanisms, we'll have what we need..

Comment 27

[Darin Fisher]

Comment on attachment 113226 [details] [diff] [review]
NS_NewBlockingInputStream

>+NS_NewBlockingInputStream(nsIInputStream** aResult,
>+                          nsIInputStream* aStream,
>+                          PRUint32 aSegmentSize = nsIUnicharStreamLoader::DEFAULT_SEGMENT_SIZE)

ok, so aStream had better be a "blocking" input stream already per
the contract of nsIStreamTransportService::CreateInputTransport, so
the name of this helper function isn't quite right.  was a nice stab
at a name though... at least it looks nice ;-)

also, this function should allow the caller to pass aSegmentSize and
aSegmentCount.	the default values should be 0 and 0.  necko treats
0 as a flag to say "use the default buffer parameters."

otherwise, this is right on!

Comment 28

[Alec Flett]

thanks - so any suggestions as to the name? NS_NewBackgroundInputStream?
NS_NewTransportInputStream? I'm pretty flexible...

Comment 29

[Darin Fisher]

maybe use "Background" more as an action than an adjective??

NS_BackgroundInputStream(nsIInputStream **result,
                         nsIInputStream *source,
                         PRUint32 segmentSize = 0,
                         PRUint32 segmentCount = 0);

NS_BackgroundOutputStream(nsIOutputStream **result,
                          nsIOutputStream *sink,
                          PRUint32 segmentSize = 0,
                          PRUint32 segmentCount = 0);

IOW these functions take a stream and operate on it in the background ;-)

Be sure to include big scary comments stating that the stream being backgrounded
 must be threadsafe!  in fact, the implementation of the stream should not count
on Read being called on any particular thread, but Read will never be called
re-entrantly.

Comment 30

[Alec Flett]

Attachment: NS_Background[Input|Output]Stream

ok, here's a patch for NS_BackgroundInputStream and one for OutputStreams as
well.

Comment 31

[Alec Flett]

now the other thing I've forgotten was where we would put the
nsIInputStream->nsIStreamListener pump? an inline version would work, but I
think it would be large. Is there an existing service that I can hang this off of?

Comment 32

[Darin Fisher]

alec: before implementing that / decided on where this should live, can you see
if this approach is even worthwhile? ;-)

Comment 33

[Alec Flett]

Comment on attachment 113440 [details] [diff] [review]
NS_Background[Input|Output]Stream

well at the very least I'd like to get these functions into the tree so we can
experiment with them.
Doug/Darin, can I get a review?

Note that I forgot the "= 0" in NS_BackgroundOutputStream in the patch, but
have the = 0 in my tree now.

Comment 34

[Darin Fisher]

Comment on attachment 113440 [details] [diff] [review]
NS_Background[Input|Output]Stream

sr=darin

Comment 35

[Alec Flett]

bumping these to 1.5alpha, I just don't have time in 1.4beta...

Comment 36

[mmc]

This bug has a patch with r+/sr+. Is it going to make it for 1.5alpha?

Comment 37

[Darin Fisher]

mc: hmm, the patch landed a while back actually... alec, you still need this bug
open for something?

Comment 38

[Peter Van der Beken [:peterv]]

(In reply to comment #26)
> 2) provide another mechanism to pump an nsIInputStream into a nsIStreamListener
> until it is empty. 

It looks like the part 2 never happened. Alec? NS_Background*Stream looks unused right now.

Comment 39

[Alec Flett]

You're asking me 4 and a half years later? I have no idea, sorry :)