99165 - Freeze nsIInputStream nsIOutputStream

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Doug Turner (:dougt)]

Darin, could you review these to interfaces and suggest any changes to these
interface before we freeze them?

* Make sure that error codes are exposed somewhere!
* There is currently an unused idl file called nsIInputStream2.idl. We might
want to remove this so that the name nsIInputStream2 is available for future
versions ot *this* interface.

Comment 1

[Darin Fisher]

dougt: no problem... taking.

Comment 2

[Darin Fisher]

Attachment: v1.0 IDL changes

Comment 3

[Darin Fisher]

dougt, brendan:

i've tweaked the input stream, output stream, and pipe interfaces a bit... and
i'd greatly appreciate your comments.  thanks!

summary of changes:

1- remove unused interface nsIPipe
2- merge nsI{In,Out}putStreamObserver into nsIPipeObserver
3- modify NS_NewPipe to accept a nsIPipeObserver parameter
4- remove observer attribute from nsI{In,Out}putStream
5- remove unused method SetNonBlocking from nsIOutputStream
6- rename GetNonBlocking -> IsNonBlocking on nsI{In,Out}putStream
7- complete comments for nsI{In,Out}putStream methods, including detailed
description of common error codes.

this IDL change means fixing up every implementation of nsI{In,Out}putStream...
which i'm willing to do.

it also means modifying code in:

  netwerk/base/src/nsStreamListenerProxy.{h,cpp}
  mailnews/mime/src/nsStreamConverter.cpp
  mailnews/mime/emitters/src/nsMimeBaseEmitter.{h,cpp}

to account for the introduction of nsIPipeObserver.

Comment 4

[Doug Turner (:dougt)]

The changes look good.  

This is problem unneeded:

+     * XXX not all implementations of nsIInputStream implement ReadSegments.

Comment 5

[Darin Fisher]

how about if i extend the comments there to read something like:

// callers of ReadSegments should fallback on calling Read, if ReadSegments 
// returns NS_ERROR_NOT_IMPLEMENTED, since not all nsIInputStream implementations
// can provide ReadSegments.

Comment 6

[Doug Turner (:dougt)]

no.  any method could return NS_ERROR_NOT_IMPLEMENTED. Why special case
ReadSegments?  

Also, are we proving too much information about the actual implemention in this
interface definition?  Just looking at the result codes, I think that we may be
locking ourselves into a paticular implemention.  

Thoughts?

Comment 7

[Darin Fisher]

the other approach to this business of input/output stream observer would be to
define interfaces like nsIObservable{In,Out}putStream, with observer attributes.
this might make more sense, since some clients only care to observe "input-side"
events. for example.  thoughts?

Comment 8

[Darin Fisher]

but ReadSegments is a special optimization of Read... it allows the caller to
get at the stream's buffer.  the problem here is that not all streams have a
buffer.  the socket transport, for example, defines an nsIInputStream that
cannot implement ReadSegments because there is no buffer... only a socket.

nsI{In,Out}putStream are two interfaces that need to be nailed down solid, and
this includes nailing down the return codes.  doing so is not overly restrictive
IMO... it is, i would say, absolutely necessary in order to define the
interface.  error codes (such as NS_BASE_STREAM_WOULD_BLOCK) are just as much a
part of the interface as the method names and parameters.  they have to be
carefully designed to avoid incorrect stream implementations.

i have had to fix _several_ ReadSegments implementations because of
inconsistencies, etc. in the way the interface was interpreted... and my point
in completely defining the interface is to avoid these problems from creeping up
again.

Comment 9

[Brendan Eich [:brendan]]

What I think, briefly:

- factor Observable{In,Out}putStream as darin proposes
- factor buffer access out, consider using nsIStreamBufferAccess (my ugly baby,
feel free to propose changes to it).
- specify nsresults carefully.

I'll promptly review a new patch!

/be

Comment 10

[Darin Fisher]

i doubt that nsIStreamBufferAccess can replace {Read,Write}Segments because the
{Read,Write}Segments implementation can decide what the segment lengths are,
whereas with nsIStreamBufferAccess, the caller gets to decide the segment lengths.

satisfying a call to getBuffer would sometimes mean allocating a new buffer of
specified length, and then doing a buffer copy, would it not?  am i
misunderstanding something?

Comment 11

[Brendan Eich [:brendan]]

I didn't mean to say that getBuffer could replace readSegments -- only that a
separate interface (ideally without adding a new interface, therefore perhaps
extending nsIStreamBufferAccess) seems like the right place for readSegments;
ditto writeSegments.

getBuffer, per the doc comment (tell me if it's unclear), should return null if
more bytes are requested than are available.

/be

Comment 12

[Darin Fisher]

> I didn't mean to say that getBuffer could replace readSegments -- only that a
> separate interface (ideally without adding a new interface, therefore perhaps
> extending nsIStreamBufferAccess) seems like the right place for readSegments;
> ditto writeSegments.

ok, that makes sense.. i should have realized that that was what you were saying :P


> getBuffer, per the doc comment (tell me if it's unclear), should return null 
> if more bytes are requested than are available.
> 
> /be

yeah.. i figured there was some way to indicate failure, but it would make it
difficult on the caller, because they'd have to know what size to ask for, or
use some algorithm to converge on a valid size.  at any rate, we agree: it's not
a replacement for {Read,Write}Segments.

Comment 13

[Darin Fisher]

here's a link to the bug which moved {Read,Write}Segments onto
nsI{In,Out}putStream and removed nsIPipeObserver in favor of SetObserver on
nsI{In,Out}putStream:

http://bugzilla.mozilla.org/show_bug.cgi?id=46777

seems like i'm wanting to basically back out the patch from that bug ;-)  [sigh]

Comment 14

[rpotts (gone)]

yep...  Originally, ReadSegments/WriteSegments were broken out into a separate 
interface :-)

I believe that they were collapsed into the nsI{In,Out}putStream interfaces to 
simplify their usage.  There were several (many) situations where it was 
difficult to deal with the dual code paths arising from this...

When, not all streams supported {Read,Write}Segments we had to create fallback 
code which was difficult to maintain and test :-(

What is the overriding reason to change this now?  Dealing with bug #46777 was 
*very* painful and it seems unclear why we want to revisit this again !!

-- rick

Comment 15

[Darin Fisher]

rick:

there are implementations of nsI{In,Out}putStream that simply cannot implement
{Read,Write}Segments (eg. consider the file and socket streams).  buffered
streams have buffers, unbuffered streams do not.

currently, we have to deal with the fact that given _a_ nsIInputStream, there
is no guarantee that it will implement ReadSegments.  this is sort of "ok"
because our code usually knows when the input stream _must_ implement
ReadSegments and it also knows when Read is just fine.  i strongly believe
that ReadSegments should be implemented whenever possible.  however, it must
be stated clearly that there is no guarantee that it will be implemented.

so, the argument for moving ReadSegments onto a separate interface is simply
that since ReadSegments is optional, callers should QI to some nsIWhatever
interface to call ReadSegments.  afterall, that is the point of QI isn't it?

the argument for keeping ReadSegments on nsIInputStream is simple: most input
streams have an underlying buffer.  only very few do not, and those that do
not are burried in the bowels of necko.  and, moreover we really want people
to implement ReadSegments because it allows us to avoid buffer copies in
generic stream handling code.

so, i'm currently supporting this latter argument, with the additional comment
that ReadSegments may not be supported if the underlying stream is not buffered.
this seems to make the most sense to me.

people need not write fallback code... assertions should be used when fallback
code is not present... we don't want people naively accepting a world without
ReadSegments ;-)

so that is the first part of this discussion.  the second part concerns the
nsI{In,Out}putStreamObserver stuff.  all implementations of nsI{In,Out}putStream
stub out the [SG]etObserver methods except for nsPipe2.  i understand that
the old API can lead to cyclic references (the pipe owns an observer which is
the owner of the pipe), but the API change didn't really solve this problem
entirely by itself... it just made it more convenient to solve the problem.
only the mailnews mime emitter code and the stream listener proxy ever make
use of the stream observers and they always know that they are dealing with
a pipe.

i'm leaning toward reinstating nsIPipeObserver because i think it yields the
cleaner api.  the cyclic reference problems are easily solved by delaying
pipe creation in the stream listener proxy case.

finally, the point of this bug is to clean up the input and output stream
definitions so we can freeze them.  i think that time has shown that some of
the methods maybe should never have been there in the first place.  for example,
no one calls nsIOutputStream::SetNonBlocking, and i think it can/should be
removed.  no one needs to call SetObserver on a nsIInputStream, so why require
that aall nsIInputStream implementations deal with the [SG]etObserver methods?

i know the work to fix all of the stream impl's is going to be much, but i'm
willing to do the work if it means that the api is actually improved.

Comment 16

[rpotts (gone)]

hey darin,

i totally agree with you...  i was just a bit scared that some of the changes
being discussed here were *not* small :-)

so, i think that the strategy that you are taking of:
 1. Keeping ReadSegments() in the base API for simplicity (and provide good
documentation)

 2. Moving the StreamObserver methods into a separate interface.

is the best option - given the situation that we're in :-)

-- rick

Comment 17

[Darin Fisher]

brendan, dougt, mscott: any (further) comments?  i'd like to move forward with
an implementation if no one objects.

Comment 18

[Doug Turner (:dougt)]

We need to move NS_NewPipe out of the IDL.  It should be put in some header
file.  We have not been freezing interfaces with C function declarations for
utility functions.  (I think that even the purist can forgive the NS_CALLBACK
declarations).

Do you really insist on this comment:

+     * XXX not all implementations of nsIOutputStream implement WriteFrom

Comment 19

[Darin Fisher]

well, implementing WriteFrom can introduce a lot of extra code if the underlying
stream is unbuffered and non-blocking, because it would impose a buffer.  so, i
think that WriteFrom should be marked with a conditional statement much like
that for WriteSegments (and ReadSegments).

Comment 20

[Darin Fisher]

does nsIPipe need to be frozen as well?  i was thinking it should be saved for
another bug/time... how urgently does it need to be frozen?

Comment 21

[Doug Turner (:dougt)]

nsIPipe has no external customers (that I know of).  Unless there is not a need
to freeze it, lets not yet.

Comment 22

[Darin Fisher]

great... patch coming.

Comment 23

[Darin Fisher]

Attachment: v1.1 revised IDL a bit

Comment 24

[Darin Fisher]

-> 0.9.6

Comment 25

[Darin Fisher]

Attachment: v1.2 yet another revision

Comment 26

[Darin Fisher]

Attachment: patch: xpcom/io changes

Comment 27

[Darin Fisher]

-> 0.9.7

Comment 28

[Darin Fisher]

bumped again -> 0.9.8

Comment 29

[Darin Fisher]

punt -> 0.9.9 

Comment 30

[Darin Fisher]

Attachment: v2 - complete patch

this patch takes the simplest route...

created nsIObservableInputStream and nsIObservableOutputStream
 - enables us to remove the |observer| attribute from nsI{In,Out}putStream.
 - we can always change the way stream observers are defined/handled later if
   we like.
 - keeps patch small and simple

Comment 31

[Darin Fisher]

Attachment: candidate nsIInputStream.idl

Comment 32

[Darin Fisher]

Attachment: candidate nsIOutputStream.idl

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 68517 [details] [diff] [review]
v2 - complete patch

Fix up the comments of isNonBlocking() such that the return values use @return.
(I don't really see a reason why client would have to know about the blocking
state of the stream...)

also fix up the available() comment in the same manner.

I am not positive about keeping the native stuff in the IDL.  I is frowned
upon, but I do not see a better way to do this.  

add @status lines for both the interface and the C callback.  The status shoudl
be marked UNDER_REVIEW.

with those changes, r=dougt.

Comment 34

[timeless]

1. why isn't available a readonly attribute?

2. can you guys wait for me to critique this over the weekend?

Comment 35

[Doug Turner (:dougt)]

1. why isn't available a readonly attribute?

--> I think that is legacy.  Also, GetAvailable() is just not as nice sounding.
 :-)  I object only on the basis that LOTS of code will have to be converted.

2. can you guys wait for me to critique this over the weekend?

--> If Darin checked in today, the interfaces will only be marked UNDER_REVIEW.
 During this period, anyone can still offer suggestings.  However, the period of
time between marking UNDER_REVIEW will be < the time until Mozilla 1.0.  So, I
guess I am saying don't sweat it if you can't review over the weekend.  You
still have a bit of time. 

Comment 36

[Darin Fisher]

Attachment: v3 - revised patch

Comment 37

[Darin Fisher]

Attachment: v3- candidate nsIInputStream.idl

Comment 38

[Darin Fisher]

Attachment: v3- candidate nsIOutputStream.idl

Comment 39

[Darin Fisher]

Attachment: v4 - revised per additional comments from dougt over AIM

Comment 40

[Darin Fisher]

Attachment: v4 - final nsIInputStream.idl

Comment 41

[Darin Fisher]

Attachment: v4 - final nsIOutputStream.idl

Comment 42

[timeless]

ok, it appears you have added new files eg
Index: xpcom/io/nsIObservableInputStream.idl
which are marked NPL and describe themselves as products of fastload. I think 
both of these are incorrect.

http://www.mozilla.org/hacking/mozilla-style-guide.html
    Use attributes wherever possible 

Whenever you are retrieving or setting a single value without any context, you 
should use attributes. Don't use two methods when you could use one attribute.

Using attributes logically connects the getting and setting of a value, and 
makes scripted code look cleaner.

Of course, that was 2001. This is 2002. Perhaps (and I'm being synical now) 
what was in style then is out of style now, and we don't care that our 
public, published, frozen interfaces are ugly.

I'm not requesting that we internally convert to use the attribute getter. 
Although I would volunteer to do the tree whacking (and brendan will complain 
that I'm wasting my time on a non 1.0 priority).

Comment 43

[Darin Fisher]

timeless: thanks for catching the license header problem.  i'll correct that.

as for Available(), normally i'd agree that it should be made an attribute, but
in this case, i think it should remain as such.  not so much because of the fact
that it would be painful to modify, but mostly because this is an interface that
is rarely seen by JS code.  afterall, what can you really do with a
nsIInputStream in JS code.  you certainly can't read from it ;-)  so, why bother
making this interface easier for JS coders when they'll never ever use it??

nsIScriptableInputStream is what JS code uses.  so, let's just leave Available()
alone, cuz it's a lot nicer for C++ coders than GetAvailable() IMO.

now if you want to make nsIScriptableInputStream::available() become a readonly
attribute, i'd support that! ;-)

Comment 44

[Alec Flett]

I personally have to agree with timeless, though not for the same reasons..
GetAvailable() is more readable to me than Available(), because the latter
sometimes seems like you're making something available or something, because
there's no verb... my $0.02
(I'm not going to say "don't land it like this!!" - I just prefer GetAvailable()
- I'm hoping to convince, rather than coerce :))

Comment 45

[Darin Fisher]

alecf: NSPR has PR_Available.  that is why nsIInputStream has Available(). 
perhaps this is not reason enough, but i think it's worth pointing out.  i'm not
going to push back to hard on this one... if people feel that this should be an
attribute, then fine... i'll make it an attribute.  anyone listening, please
weigh in with your opinion.  thx!

Comment 46

[Doug Turner (:dougt)]

Or we could be verbose and call it
GetAvailableByteCountInTheStreamThatCanBeReadWithoutBlocking(). :-)
Java also treats this as a function named "available".  

My 2 cents: we have lived with it this long, might as well leave it.  

Comment 47

[Alec Flett]

Java and NSPR seem like reasonable enough reasons to leave it :)

Comment 48

[Brendan Eich [:brendan]]

Let's save our effort for higher priority 1.0 work, and stick with the Java
precedent warren followed years ago.

/be

Comment 49

[timeless]

nc4 spellcheck indicates you use 
* The signature for the reader function passed to WriteSegment. This 
...
* @param closure - opaque parameter passed to WriteSegments

Note that there is no WriteSegment method in the idl

* @param count - the maximun number of bytes to write
maximun isn't a word.
[note: imgIDecoder.idl also contains this line]
--End Spell Checker output for attachment 69603 [details] and attachment 69604 [details]

* @return count read if successful (0 if reached EOF)
Perhaps 'return the number of bytes read'.  I think the parenthetical is 
ambiguous supposing I read two characters LF EOF, do I return 0 [if reached 
EOF]? You meant return 0 if the buffer was at EOF before the function call.

* The signature of the writer function passed to ReadSegments. This
* specifies where the data should go that gets read from the buffer.
:-(
This specifies where data that gets read from the buffer should go.

* @param toOffset - amount already read (starting from zero)
I don't like 'starting from zero', I think zero based/indexed, starting at/with 
or something similar might be better. However this is not a deal breaker :-)

* @return NS_BASE_STREAM_WOULD_BLOCK if done writing
? please explain what you mean.

* Errors are passed to the caller of ReadSegments, unless a previous call to 
* the writer function resulted in some data being written.
Please clarify that 'unless' only covers a single call to ReadSegments.

* @param buf - the buffer into which the data is read
I personally don't like 'is' and would prefer one of  'will be', 'to be' or 
'should be' however I could be out of bounds here (or elsewhere ...)

I've added http://java.sun.com/j2se/javadoc/writingdoccomments/index.html to my 
reading list, but haven't read it yet :-(

"<or>" doesn't appear in any current idl files and a quick buggy search didn't 
turn it up in the javadoc reference, I'd suggest removing it.

ooh, spiffy nit from the writingdoccomments site:
      Avoid Latin -- use "also known as" instead of "aka", use "that is" or "to 
be specific" instead of "i.e.", use "for example" instead of "e.g.", and use 
"in other words" or "namely" instead of "viz." 

Note that even if you don't replace "eg." with "for example", you should 
replace it with "e.g." -- There are currently 8 violators of this in dist\idl 
(and just shy of 40 users of "e.g." -- two are frozen), there are at least 25 
instances of "for example" in dist\include. None of the 8 "eg." files have a 
@status and I will attempt to request the same correction there in the event I 
encounter those files in a bug report. -- Or I might just whack them in my 
spelling bug.

[usage of latin ie in dist\idl is actually a clearer case: 6 ie. 32 i.e., 52 
that is]

Comment 50

[Darin Fisher]

-> 1.0

Comment 51

[Darin Fisher]

Attachment: v5 revised per comments from timeless

Comment 52

[Darin Fisher]

Comment on attachment 72105 [details] [diff] [review]
v5 revised per comments from timeless

r=dougt (verbal)

Comment 53

[Darin Fisher]

Attachment: v5 nsIInputStream.idl

Comment 54

[Darin Fisher]

Attachment: v5 nsIOutputStream.idl

Comment 55

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72105 [details] [diff] [review]
v5 revised per comments from timeless

>+++ embedding/browser/gtk/src/EmbedStream.cpp	1 Mar 2002 19:40:48 -0000
>@@ -271,6 +271,10 @@
>   if (NS_SUCCEEDED(rv)) {
>     PRUint32 writeCount = 0;
>     rv = aWriter(this, aClosure, readBuf, 0, nBytes, &writeCount);
>+
>+    // XXX writeCount may be less than nBytes!!  This is the wrong
>+    // way to synthesize ReadSegments.
>+    NS_ASSERTION(writeCount == nBytes, "data loss");
>   }

File a bug?  Shouldn't it just return NS_NOT_IMPLEMENTED if it's not going
to go the whole way with ReadSegments?

>+EmbedStream::IsNonBlocking(PRBool *aNonBlocking)
> {
>-    NS_NOTREACHED("SetObserver");
>+    NS_NOTREACHED("IsNonBlocking");
>     return NS_ERROR_NOT_IMPLEMENTED;
> }

This seems odd to me.  Is the EmbedStream in an indeterminate state, and
can't know if it's blocking or not?  In that case, I would report that it
is not-non-blocking (which is to say, it's not un-anti-counter-blocking)
because it might cause a caller to block at some point, if the cat turns
out to be dead and the EmbedStream is blocking.  This -- and the many
others you touched but did not write -- seem to be non-conformant, and
need to be cleaned for 1.0 if this interface is to be frozen.  Can you
either fix them or file bugs on their owners?

>+    // we no longer need to be the default load request
>+    /*
>+    if (mLoadGroup && (mLoadFlags & LOAD_DOCUMENT_URI)) {
>+        rv = mLoadGroup->SetDefaultLoadRequest(newChannel); 
>+        if (NS_FAILED(rv)) return rv;
>+    }
>+    */
>+

Tear it out!  If we need old versions, we have CVS.

>+     * @param aReader the "provider" of the data to write

Is "to be written" better?

>+     * @return count written

"number of bytes", for symmetry with @param aCount.

(Man, we have a lot of different stream types.	 Do we really use
them all?)

I like the interfaces, generally, and I agree with calling out the
optional-implementability of ReadSegments: QIing to an interface
successfully is accepting a contract to implement that interface
to its specified fullest.  Where the interface wishes to make a
method optional, for valid pragmatic reasons as we see here, I
think documenting the validity of an NS_ERROR_NOT_IMPLEMENTED
return is a pretty good option.

This stuff is just going UNDER_REVIEW, right?  Marking sr, because
I think this stuff is fine to check in now (modulo comment-block
removal and language nits, which I don't need to see in another
patch), but please do file or fix the non-conformant implementations
in our tree.

Comment 56

[Asa Dotzler [:asa]]

Comment on attachment 72105 [details] [diff] [review]
v5 revised per comments from timeless

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

sorry for coming so late in the game (too late even). But is there a reason that
nsIObservableInputStream does not inherit nsIInputStream? It seems like it would
be easier to use (less QI'ing for someone that uses the observer) and saves a
few bytes in the implementations (though that might be very little).

Anyhow, this isn't very important to me, so if this is too late then don't worry

Comment 58

[Darin Fisher]

sicking: i thought about that too, but consider this:

  interface nsIA : nsIInputStream
  { };
  interface nsIB : nsIInputStream
  { };
  class AB : nsIA, nsIB
  { };

now class AB will have to use ugly casts to get to a nsIInputStream pointer:

  nsIInputStream *in = (nsIInputStream*)(nsIA*)this;

and class AB will have to use an interface map w/
 
  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS

macros mixed in...

not that that's so terrible, but it just doesn't seem like there are enough
cases where someone will want to frequently call [GS]etObserver and the
nsI{In,Out}putStream methods.  I could be wrong about that, but the examples in
our code right now don't indicate any need to hold onto
nsIObservable{In,Out}putStream pointers and call the nsI{In,Out}putStream
methods directly on those :-/

also, the pipe constructor returns nsI{In,Out}putStream pointers, so consumers
have to QI to nsIObservable{In,Out}putStream anyways... though i suppose the
pipe constructor could return the observable interface if it inherited from the
base stream interfaces.  hmm...

Comment 59

[Darin Fisher]

v5 patch (+modifications suggested by shaver) landed on trunk.

keeping this bug open to track final tweaks (if required) + final switch from
UNDER_REVIEW to FROZEN.

Comment 60

[Darin Fisher]

actually, resolving this as FIXED... i'll stamp FROZEN on these interfaces along
w/ various necko interfaces for bug 124465.