Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10836177:
Add StreamBuffer to dart:io. (Closed)
|
Created:
8 years, 4 months ago by Bill Hesse Modified:
7 years, 11 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd StreamBuffer to dart:io.
BUG=
Patch Set 1 #
Total comments: 21
Patch Set 2 : Rebase, without changes. #
Messages
Total messages: 8 (0 generated)
Not quite finished (writeString, and more tests are needed), but here is a StreamBuffer class that I think could replace ListInputStream and ListOutputStream effectively. Do you need more control flow and throttling than this provides?
http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart File runtime/bin/stream_buffer.dart (right): http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:19: toBuffer._buffer = this; Style nit: I think this should be intended only four spaces. http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:35: fromBuffer._onClosedScheduledEvent.cancel(); As I mentioned in issue 4222, I don't understand why [destroy] doesn't trigger onClosed. If one end is destroyed, how does the other know to stop writing/reading? http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:40: final _StreamBufferOutputStream toBuffer; For tooling/documentation reasons, the types of the public members should probably also be public. It feels a little silly, but the right way to do this is probably to make these fields private and add public getters that have the public types. http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:41: final List<List<int>> _data; Would it be easier to make this a [_BufferList]? It seems like that would make [read] and [readInto] much cleaner, as well as allowing you to avoid manually tracking [_available]. http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:48: class _StreamBufferInputStream implements InputStream { Why not extend [_BaseDataInputStream]? http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:50: bool get closed() => _buffer._closed; I'm a little confused how this relates to what you wrote in issue 4222. Won't [closed] be true as soon as [close] is called, as opposed to after [onClosed] has fired? http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:64: "Error events not supported on StreamBuffer"); It would be great if errors were supported (see issue 3657). Maybe [StreamBuffer.reportError]? Bonus points if it's also possible to pass a stack trace along with the reported error. http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:142: void pipe(OutputStream output, [bool close]) { Don't you get this for free from [_pipe] in stream_util.dart? http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:158: } Extra closing brace http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:183: Remove empty lines http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:189: throw new StreamException("Writing to a destroyed StreamBuffer"); Is it useful to distinguish this error from the stream being closed? http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:227: void set onClosed(void callback()) { The behavior of this callback doesn't match the description you gave for my CL. Currently, this will only get called if the StreamBuffer is destroyed. According to the description, though, it should also be called when the underlying communication channel has been closed, that is to say when [close] has been called. http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:236: void set onNoPendingWrites(void callback()) { It seems like this should have some implementation, even if it's trivial (e.g. being called over and over again until the stream is closed). I can imagine an API that takes an OutputStream and relies on onNoPendingWrites to figure out when it's acceptable to write to it. We'd want that API to work with _StreamBufferOutputStream, even if it just ends up being equivalent to writing everything at once. http://codereview.chromium.org/10836177/diff/1/runtime/bin/stream_buffer.dart... runtime/bin/stream_buffer.dart:249: } Now that my [OutputStream.closed] CL is submitted, this should also support that getter.
https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... File runtime/bin/stream_buffer.dart (right): https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:11: StreamBuffer() I think the Dart style calls for four space indent of ':' here. https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:35: fromBuffer._onClosedScheduledEvent.cancel(); On 2012/08/09 22:03:45, nweiz wrote: > As I mentioned in issue 4222, I don't understand why [destroy] doesn't trigger > onClosed. If one end is destroyed, how does the other know to stop > writing/reading? Shouldn't it call onError? Destroy sounds like just killing the whole thing. In normal situations you don't need destroy a assume. https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:41: final List<List<int>> _data; On 2012/08/09 22:03:45, nweiz wrote: > Would it be easier to make this a [_BufferList]? It seems like that would make > [read] and [readInto] much cleaner, as well as allowing you to avoid manually > tracking [_available]. I agree _BufferList was made to fit this purpose. Feel free to add more to _BufferList as needed.
Great comments. I will use _BufferList, but it needs a lot of extension to have the right functionality. I'll check the BaseDataInputStream and BaseOutputStream class - they should do what we want. The semantics of the API should be pinned down. I'm beginning to see the point that we should have a "normal" way of shutting down an input stream from the receiver - close(), that is not an error, and that an output stream should get notified of this. OnNoPendingWrites is an event that I thought would only be enabled after a "failed" write - one that returns true. That is what we do with onWrite on the socket - it is only fired after a partial write, and cleared afterwards. In any case, it should have no meaning on a pipe - I don't think we should be firing it. The flow control on an output stream is only advisory, and we want the onNoPendingWrites event when the condition changes from "no writes desired at the moment" to normal.
On 2012/08/10 09:31:07, Bill Hesse wrote: > OnNoPendingWrites is an event that I thought would only be enabled after a > "failed" write - one that returns true. That is what we do with onWrite on the > socket - it is only fired after a partial write, and cleared afterwards. In any > case, it should have no meaning on a pipe - I don't think we should be firing > it. The flow control on an output stream is only advisory, and we want the > onNoPendingWrites event when the condition changes from "no writes desired at > the moment" to normal. According to the documentation of onNoPendingWrites, it should be called whenever there's no data that's been written to the output stream but not yet written to the underlying channel, which is different from what you're saying here. If that documentation is wrong, it should be updated.
What's the status on this CL? I'm eagerly awaiting StreamBuffer.
I didn't realize anyone was enthusiatic about it - It needs some work, but I'll bump its priority. https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... File runtime/bin/stream_buffer.dart (right): https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:50: bool get closed() => _buffer._closed; On 2012/08/09 22:03:45, nweiz wrote: > I'm a little confused how this relates to what you wrote in issue 4222. Won't > [closed] be true as soon as [close] is called, as opposed to after [onClosed] When close() is called on the output stream, _closePending is set. _close is only set once the onClose callback on the input stream is called. When close() is called on the input stream, then everything is shut down, and I was thinking onClose shouldn't be called because this is done by the input stream, so it should know that it is closed. But I could be persuaded that close() on the input stream and destroy() on the output stream should trigger an onClose event. > has fired? https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:158: } On 2012/08/09 22:03:45, nweiz wrote: > Extra closing brace Actually, wrong indentation.
Thanks for picking this up again. It'll be helpful for our Pub work, especially if it supports error propagation. https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... File runtime/bin/stream_buffer.dart (right): https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:35: fromBuffer._onClosedScheduledEvent.cancel(); On 2012/08/10 07:58:10, Søren Gjesse wrote: > On 2012/08/09 22:03:45, nweiz wrote: > > As I mentioned in issue 4222, I don't understand why [destroy] doesn't trigger > > onClosed. If one end is destroyed, how does the other know to stop > > writing/reading? > > Shouldn't it call onError? Destroy sounds like just killing the whole thing. In > normal situations you don't need destroy a assume. That's not how InputStream#close or OutputStream#destroy work in other implementations. https://chromiumcodereview.appspot.com/10836177/diff/1/runtime/bin/stream_buf... runtime/bin/stream_buffer.dart:50: bool get closed() => _buffer._closed; On 2012/09/04 23:17:13, Bill Hesse wrote: > On 2012/08/09 22:03:45, nweiz wrote: > > I'm a little confused how this relates to what you wrote in issue 4222. Won't > > [closed] be true as soon as [close] is called, as opposed to after [onClosed] > > When close() is called on the output stream, _closePending is set. _close is > only set once the onClose callback on the input stream is called. > > When close() is called on the input stream, then everything is shut down, and I > was thinking onClose shouldn't be called because this is done by the input > stream, so it should know that it is closed. But I could be persuaded that > close() on the input stream and destroy() on the output stream should trigger an > onClose event. > > has fired? > It seems to be that (for an InputStream) exactly one of [onClosed] or [onError] should fire before [closed] becomes true in all cases. It's true that the code calling [InputStream.close] will usually be the same as the code that would receive the event, but there will be cases where that code returns the InputStream to other code as well (especially when issue 4202 is fixed). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
