Skip to content

Should Writer have a flush method? #9126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anasazi opened this issue Sep 11, 2013 · 12 comments · Fixed by #10180
Closed

Should Writer have a flush method? #9126

anasazi opened this issue Sep 11, 2013 · 12 comments · Fixed by #10180
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@anasazi
Copy link
Contributor

anasazi commented Sep 11, 2013

Currently, rt::io::Writer requires a method flush(&mut self) that flushes output. However, this does not make sense for every implementation of Writer. Specifically, libuv has no notion of flush, so none of the current I/O can do it anyway. Some implementations fail! when called (TcpStream, UdpSocket) while others do nothing at all (FileStream).

Rather than unifying the I/O system on the fail! option or the no-op option, I'd like to suggest removing flush from Writer and creating a trait Flushable that extends Writer. We really shouldn't be implementing methods where they don't make sense when we can do otherwise.

@anasazi
Copy link
Contributor Author

anasazi commented Sep 11, 2013

@brson
Copy link
Contributor

brson commented Sep 11, 2013

I wasn't sure about this when I declared it, but one thing I've noticed about I/O libraries is that the abstractions tend to be imperfect like this. I think I recall copying this decision from .NET, but I'm ok trying to move flush to its own trait. If we do that I don't think it necessarily needs to extend Writer though.

My inclination though is that having a no-op flush is fine and convenient. Without the no-op flush, Writers that don't do buffering can't be used with code that expects to be able to flush.

@alexcrichton
Copy link
Member

I think that everything calling fail!() today is just a relic of having been not-implemented at the time of being written, so I don't think that anything ever intended to actually fail.

Other than that, I'm a little bit in favor of having flush on the Writer trait because I always fear trait-explosion problems. That being said, I wouldn't be opposed at all to having a Flush trait. I'd agree with @brson that it probably doesn't need to extend Writer.

One problem that should be addressed though: If a BufferedWriter has a flushable underlying Writer, when the buffered version is flushed the writer should also be flushed. Otherwise, if the underlying writer isn't flushable, just the buffered writer should be flushed. I don't think that this would be possible if we extract flush from Writer (although I could be wrong).

@thestinger
Copy link
Contributor

@brson: it's a perfect use case for a default method, with a no-op as the default

@anasazi
Copy link
Contributor Author

anasazi commented Sep 11, 2013

I'd rather make it clear that flush doesn't do anything so you shouldn't bother calling it.

I'm not really sure why you would want a Flushable trait to not extend Writer. What can be flushed that can't be written to?

I'd rather have BufferedWriter expect a Flushable and combine it with a newtype wrapper for Writer that does a no-op flush. That way it's explicit that flushing doesn't make sense on the underlying Writer and you're telling BufferedWriter to just do nothing instead.

@anasazi
Copy link
Contributor Author

anasazi commented Sep 11, 2013

trait Writer {
    fn write(&mut self, buf: &[u8]);
}

trait Flushable : Writer {
    fn flush(&mut self);
}

struct IgnoreFlush<T>(T);

impl<T: Writer> Writer for T {
    fn write(&mut self, buf: &[u8]) {
        (**self).write(buf)
    }
}

impl<T: Writer> Flushable for T {
     fn flush(&mut self) { /* do nothing */ }
}

Something like this. BufferedWriter would impl both Writer and Flushable.
Rather than BufferedWriter::new(some_tcp_stream), you'd do BufferedWriter::new(IgnoreFlush(some_tcp_stream)).

@thestinger
Copy link
Contributor

I think it would be much simpler to just have a default method flush, because generic code using the traits is only interested in the guarantee that the internal buffers are flushed which an unbuffered writer already provides. The flush method will be implemented on files but won't provide any guarantee of syncing to disk, just that if the application is killed it won't be lost.

@anasazi
Copy link
Contributor Author

anasazi commented Sep 11, 2013

Separating them will give generic code the ability to differentiate between unbuffered writers and buffered writers, which could be useful. It basically assumes all I/O is buffered at the moment.

@sfackler
Copy link
Member

I would think that pulling flush out to a separate trait would actually hurt the ability to write generic code, at least if a FlushableWriter or whatever extends Writer. Code written to work with a Writer will not work properly if it's called with a BufferedWriter since it won't flush.

I think better options would be to either have a default no-op flush implementation, or have completely separate FlushableWriter and Writer traits with adapter structs to turn a Writer into a FlushableWriter and vice versa.

@kud1ing
Copy link

kud1ing commented Sep 12, 2013

FWIW Go defines flush() on their buffered Writer only: http://golang.org/pkg/bufio/
Their non-buffered interfaces are quite fine-grained: http://golang.org/pkg/io/

Where do we the see the need for a flush trait?

@olsonjeffery
Copy link
Contributor

So there was a PR/issue somewhere that mentions pulling in chris morgan's BufferedStream from rust-http. In there, I argued for splitting out a Buffered and Unbuffered stream and pushing those details down into the RtioFoo traits.. I think this discussion dovetails with that..

tl;dr -- i want to push Seek, flush, etc and all of that out of an UnbufferedStream (which basically would/behave like an fd accessed only w/ pread/pwrite) .. and we can do Seek, flush, etc in a BufferedStream, whose impl would be IO backend-specific..

@dckc
Copy link
Contributor

dckc commented Oct 29, 2013

FWIW, Wr from Modula-3 has flush. The M3 interfaces for systems programming seem particularly well-thought-out and well-specified, though they're designed for use with threads, locks, and exceptions, which might make them not so relevant.

flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 11, 2022
`explicit_auto_deref` changes

fixes rust-lang#9123
fixes rust-lang#9109
fixes rust-lang#9143
fixes rust-lang#9101

This avoid suggesting code which hits a rustc bug. Basically `&{x}` won't use auto-deref if the target type is `Sized`.

changelog: Don't suggest using auto deref for block expressions when the target type is `Sized`
changelog: Include the borrow in the suggestion for `explicit_auto_deref`
changelog: Don't lint `explicit_auto_deref` on `dyn Trait` return
changelog: Don't lint `explicit_auto_deref` when other adjustments are required
changelog: Lint `explicit_auto_deref` in implicit return positions for closures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants