Closed Bug 99165 Opened 23 years ago Closed 23 years ago

Freeze nsIInputStream nsIOutputStream

Categories

(Core :: XPCOM, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: dougt, Assigned: darin.moz)

References

Details

(Keywords: arch, topembed)

Attachments

(3 files, 13 obsolete files)

87.56 KB, patch
darin.moz
: review+
shaver
: superreview+
Details | Diff | Splinter Review
5.13 KB, text/plain
Details
6.25 KB, text/plain
Details
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.
dougt: no problem... taking.
Assignee: dougt → darin
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla0.9.5
Attached patch v1.0 IDL changes (obsolete) — Splinter Review
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.
The changes look good. This is problem unneeded: + * XXX not all implementations of nsIInputStream implement ReadSegments.
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.
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?
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?
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.
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
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?
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
> 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.
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]
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
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.
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
brendan, dougt, mscott: any (further) comments? i'd like to move forward with an implementation if no one objects.
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
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).
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?
nsIPipe has no external customers (that I know of). Unless there is not a need to freeze it, lets not yet.
great... patch coming.
Attached patch v1.1 revised IDL a bit (obsolete) — Splinter Review
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Attached patch v1.2 yet another revision (obsolete) — Splinter Review
Attached patch patch: xpcom/io changes (obsolete) — Splinter Review
-> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Keywords: arch
bumped again -> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Keywords: mozilla0.9.8
punt -> 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Keywords: mozilla0.9.8mozilla1.0
Attached patch v2 - complete patch (obsolete) — Splinter Review
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
Attachment #49798 - Attachment is obsolete: true
Attachment #51358 - Attachment is obsolete: true
Attachment #55279 - Attachment is obsolete: true
Attachment #55338 - Attachment is obsolete: true
Keywords: patch
Blocks: 98278
Keywords: nsbeta1+
Attached file candidate nsIInputStream.idl (obsolete) —
Attached file candidate nsIOutputStream.idl (obsolete) —
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.
Attachment #68517 - Flags: review+
1. why isn't available a readonly attribute? 2. can you guys wait for me to critique this over the weekend?
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.
Attached patch v3 - revised patch (obsolete) — Splinter Review
Attachment #68517 - Attachment is obsolete: true
Attachment #69285 - Attachment is obsolete: true
Attachment #69286 - Attachment is obsolete: true
Attached file v3- candidate nsIInputStream.idl (obsolete) —
Attached file v3- candidate nsIOutputStream.idl (obsolete) —
Attachment #69562 - Attachment is obsolete: true
Attachment #69563 - Attachment is obsolete: true
Attachment #69564 - Attachment is obsolete: true
Attached file v4 - final nsIInputStream.idl (obsolete) —
Attached file v4 - final nsIOutputStream.idl (obsolete) —
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).
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! ;-)
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 :))
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!
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.
Java and NSPR seem like reasonable enough reasons to leave it :)
Let's save our effort for higher priority 1.0 work, and stick with the Java precedent warren followed years ago. /be
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]
Keywords: topembed
Keywords: mozilla1.0+
Keywords: mozilla1.0
-> 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Attachment #69602 - Attachment is obsolete: true
Comment on attachment 72105 [details] [diff] [review] v5 revised per comments from timeless r=dougt (verbal)
Attachment #72105 - Flags: review+
Attached file v5 nsIInputStream.idl
Attachment #69603 - Attachment is obsolete: true
Attached file v5 nsIOutputStream.idl
Attachment #69604 - Attachment is obsolete: true
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.
Attachment #72105 - Flags: superreview+
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
Attachment #72105 - Flags: approval+
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
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...
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.
actually, resolving this as FIXED... i'll stamp FROZEN on these interfaces along w/ various necko interfaces for bug 124465.
Blocks: 124465
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
No longer blocks: 124465
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: