Add method flush to output stream

Add method flush to output stream

R=ager@google.com

BUG=dart:1407
TEST=tests/standalone/src/io/FileOutputStreamTest.dart

Committed: https://code.google.com/p/dart/source/detail?r=8422

[Mads Ager (google), 2012-04-17 11:56:44]

http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart#newc...
runtime/bin/file_impl.dart:164: _file.flush();
Isn't _file a RandomAccessFile? Doesn't flush take a callback argument?

http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart
File runtime/bin/output_stream.dart (right):

http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:58: void flush();
Shouldn't there be some sort of notification that the stream has actually been
flushed?

If you do:

stream.write(data);
stream.flush();

It seems to me that you are no wiser than if you had just done the write alone?

Maybe the flush call should either count as a write and onNoPendingWrites should
be called or there should be a callback here?

If we go for a callback I think we should update the documentation for
onNoPendingWrites. It says that it is called when the internal OS buffers have
been flushed. We need to make it clear what the relationship between flush and
onNoPendingWrites is.

[Søren Gjesse, 2012-04-17 13:14:19]

This is probably a file output stream only thing as on the other output streams
flush does not make much sense.

We could leave it out.

http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart#newc...
runtime/bin/file_impl.dart:164: _file.flush();
On 2012/04/17 11:56:44, Mads Ager wrote:
> Isn't _file a RandomAccessFile? Doesn't flush take a callback argument?

It does - not sure why no tests failed.

Turend out my _PendingOperation sentinels where not different, and there was no
onError handler in the test.

http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart
File runtime/bin/output_stream.dart (right):

http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart#...
runtime/bin/output_stream.dart:58: void flush();
On 2012/04/17 11:56:44, Mads Ager wrote:
> Shouldn't there be some sort of notification that the stream has actually been
> flushed?
> 
> If you do:
> 
> stream.write(data);
> stream.flush();
> 
> It seems to me that you are no wiser than if you had just done the write
alone?
> 
> Maybe the flush call should either count as a write and onNoPendingWrites
should
> be called or there should be a callback here?
> 
> If we go for a callback I think we should update the documentation for
> onNoPendingWrites. It says that it is called when the internal OS buffers have
> been flushed. We need to make it clear what the relationship between flush and
> onNoPendingWrites is.

The only reason for having flush is that even when there you get a
noNoPendingWrites callback the data written might still be sitting in an
internal buffer. This is only an issue for files where there is the possibility
that the OS will hold data in memory for some time.

If you do

stream.write(data);
stream.flush();

Then you are somewhat wiser in that, for a file, data will be on the disk "very
soon" and for a non file nothing happens.

[Mads Ager (google), 2012-04-17 13:22:02]

On 2012/04/17 13:14:19, Søren Gjesse wrote:
> This is probably a file output stream only thing as on the other output
streams
> flush does not make much sense.
> 
> We could leave it out.
> 
> http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart
> File runtime/bin/file_impl.dart (right):
> 
>
http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart#newc...
> runtime/bin/file_impl.dart:164: _file.flush();
> On 2012/04/17 11:56:44, Mads Ager wrote:
> > Isn't _file a RandomAccessFile? Doesn't flush take a callback argument?
> 
> It does - not sure why no tests failed.
> 
> Turend out my _PendingOperation sentinels where not different, and there was
no
> onError handler in the test.
> 
> http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart
> File runtime/bin/output_stream.dart (right):
> 
>
http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart#...
> runtime/bin/output_stream.dart:58: void flush();
> On 2012/04/17 11:56:44, Mads Ager wrote:
> > Shouldn't there be some sort of notification that the stream has actually
been
> > flushed?
> > 
> > If you do:
> > 
> > stream.write(data);
> > stream.flush();
> > 
> > It seems to me that you are no wiser than if you had just done the write
> alone?
> > 
> > Maybe the flush call should either count as a write and onNoPendingWrites
> should
> > be called or there should be a callback here?
> > 
> > If we go for a callback I think we should update the documentation for
> > onNoPendingWrites. It says that it is called when the internal OS buffers
have
> > been flushed. We need to make it clear what the relationship between flush
and
> > onNoPendingWrites is.
> 
> The only reason for having flush is that even when there you get a
> noNoPendingWrites callback the data written might still be sitting in an
> internal buffer. This is only an issue for files where there is the
possibility
> that the OS will hold data in memory for some time.
>
> If you do
> 
> stream.write(data);
> stream.flush();
> 
> Then you are somewhat wiser in that, for a file, data will be on the disk
"very
> soon" and for a non file nothing happens.


Wouldn't it be clearer to add a callback that gets called when the data *has*
been flushed to the file? I would be fine with leaving this out as well. What
was the use case for the bug report? I can't come up with a use case where
knowing that something will be flushed soon is useful. Knowing that it has been
flushed probably is.

It might make sense to update to comment on onNoPendingWrites to be more clear
about the 'internal buffers'? The comment explicitly says OS buffers and 'flush'
which makes it sound weird that you don't know that the output has been flushed
to the file for FileOutputStreams.

[Søren Gjesse, 2012-04-17 13:42:35]

On 2012/04/17 13:22:02, Mads Ager wrote:
> On 2012/04/17 13:14:19, Søren Gjesse wrote:
> > This is probably a file output stream only thing as on the other output
> streams
> > flush does not make much sense.
> > 
> > We could leave it out.
> > 
> > http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart
> > File runtime/bin/file_impl.dart (right):
> > 
> >
>
http://codereview.chromium.org/9969202/diff/1/runtime/bin/file_impl.dart#newc...
> > runtime/bin/file_impl.dart:164: _file.flush();
> > On 2012/04/17 11:56:44, Mads Ager wrote:
> > > Isn't _file a RandomAccessFile? Doesn't flush take a callback argument?
> > 
> > It does - not sure why no tests failed.
> > 
> > Turend out my _PendingOperation sentinels where not different, and there was
> no
> > onError handler in the test.
> > 
> > http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart
> > File runtime/bin/output_stream.dart (right):
> > 
> >
>
http://codereview.chromium.org/9969202/diff/1/runtime/bin/output_stream.dart#...
> > runtime/bin/output_stream.dart:58: void flush();
> > On 2012/04/17 11:56:44, Mads Ager wrote:
> > > Shouldn't there be some sort of notification that the stream has actually
> been
> > > flushed?
> > > 
> > > If you do:
> > > 
> > > stream.write(data);
> > > stream.flush();
> > > 
> > > It seems to me that you are no wiser than if you had just done the write
> > alone?
> > > 
> > > Maybe the flush call should either count as a write and onNoPendingWrites
> > should
> > > be called or there should be a callback here?
> > > 
> > > If we go for a callback I think we should update the documentation for
> > > onNoPendingWrites. It says that it is called when the internal OS buffers
> have
> > > been flushed. We need to make it clear what the relationship between flush
> and
> > > onNoPendingWrites is.
> > 
> > The only reason for having flush is that even when there you get a
> > noNoPendingWrites callback the data written might still be sitting in an
> > internal buffer. This is only an issue for files where there is the
> possibility
> > that the OS will hold data in memory for some time.
> >
> > If you do
> > 
> > stream.write(data);
> > stream.flush();
> > 
> > Then you are somewhat wiser in that, for a file, data will be on the disk
> "very
> > soon" and for a non file nothing happens.
> 
> 
> Wouldn't it be clearer to add a callback that gets called when the data *has*
> been flushed to the file? I would be fine with leaving this out as well. What
> was the use case for the bug report? I can't come up with a use case where
> knowing that something will be flushed soon is useful. Knowing that it has
been
> flushed probably is.
> 
> It might make sense to update to comment on onNoPendingWrites to be more clear
> about the 'internal buffers'? The comment explicitly says OS buffers and
'flush'
> which makes it sound weird that you don't know that the output has been
flushed
> to the file for FileOutputStreams.

The main use case for this is (I think) writing some kind of log filer which is
being watched by some other program. Then flush will make the data visible to
the watcher, but a callback to the caller ns not needed.

As discussed offline maybe this is to file specific to be included on the
OutputStream interface.

[Mads Ager (google), 2012-06-08 07:14:54]

LGTM. Let's land this.