Flush stdout on read from stdin again

[nagisa]

This is possible now since we have reentrant mutexes. Fixes #25555.

[rust-highfive]

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

[sfackler]

Minor nit: let _ = stdout().flush() is a bit more common than drop(stdout().flush()).

[aturon]

@nagisa Sorry for the long delay on this review! I have a few concerns.

What's the rationale for restricting this change to the read function only? Presumably other methods of reading should behave similarly?

Have you thought much about the potential for deadlock, given that you can actually acquire the lock for stdout separately, but that lock is also required to flush it? I.e., I'm worried about a situation where one thread is holding the lock for stdout, and another thread tries to read from stdin -- if those threads are also communicating with each other, there is potential for deadlock, which may be somewhat surprising if you don't realize that reading from stdin requires lock acquisition for stdout.

I'm not sure that's a blocker per se, but definitely seems like a concern.

[nagisa]

Minor nit: let _ = stdout().flush() is a bit more common than drop(stdout().flush()).

Note, that all this patch does is really reverting #23087.

What's the rationale for restricting this change to the read function only? Presumably other methods of reading should behave similarly?

Similarly, this is just a revert of functionality available previously. I believe @alexcrichton would know better about why only read was flushing.

Have you thought much about the potential for deadlock, given that you can actually acquire the lock for stdout separately, but that lock is also required to flush it?

Deadlock is indeed a possibility and I don’t see us ever being able to avoid it fully if we want to implement something like this. Before it was very easy to cause the deadlock, which was a strong motivator to remove flushing; People are having issues with status quo now and might actually prefer occasional deadlocks to lack of hand holding.

[alexcrichton]

I believe @alexcrichton would know better about why only read was flushing.

One key difference is that it was previously flushing inside of StdinLock instead of Stdin, so this PR won't actually flush unless you call read specifically on Stdin, e.g. not read_to_end, read_line, etc.

Deadlock is indeed a possibility and I don’t see us ever being able to avoid it fully if we want to implement something like this.

I agree, and I personally also viewed the removal in #23087 as a stance that "we're not ever going to do this" as silent locking behind the scenes is inevitably going to trip some up and be difficult to debug. My own opinion is that this is not what the standard library should be doing, and instead it needs to be (and maybe already is) clearly documented about the buffering that the standard input/output streams do so users are aware of the need to flush.

I would not be ready to say that 100% of users of Stdin want to flush stdout whenever it's read, which is what makes me wary of this.

[aturon]

@alexcrichton

The main counterarguments are:

• The lack of flushing is much more likely to be a footgun than the deadlock issue, since it's much more rare to explicitly take stdio locks, and we can clearly document the interaction with flushing. I believe that this flushing behavior is also pretty common in other languages, so not doing it may break people's expectations.
• We already have a fair amount of "magic" going on here just in providing built-in locks, etc. So it's not a huge departure to also bake in a flush. Though it is a bit concerning if there's no way to read without instigating a flush.

I'm pretty on the fence about this one. Would be great to hear from others in @rust-lang/libs!

[BurntSushi]

I'm more inclined for the conservative approach here: don't flush stdout when reading from stdin. It seems incredibly surprising to me that this would be default behavior.

I do recognize that not flushing can also lead to surprising behavior based on experience with other languages. But I think Rust leans more toward the explicit end of things and flushing stdout when reading stdin leans more toward the implicit end of things IMO.

[sfackler]

We could avoid the deadlock issues by having one reentrant lock around both stdin and stdout (might as well throw stderr in as well at that point). I don't think it'd cause contention issues in reasonable code but I guess it's possible?

[alexcrichton]

The idea of only one reentrant lock actually isn't so bad I think. I'm still a little uncomfortable with the reads-always-flush-no-matter-what behavior, but I believe that I could come to live with it as once you've already got a lock the overhead should be fairly negligible.

[alexcrichton]

It's looking like the consensus here is leaning more towards not taking this change, largely because of the surprising locking behavior. It's also unclear how much the standard library should be doing in this regard in terms of being as "systemsy" as possible where each operation has a clear implementation (after assuming the buffering/locking necessary, however). Additionally, the use case this is fixing doesn't quite seem to be worth the cost.

I also think that @sfackler's idea for one global lock can also lead to surprising deadlocks, for example if one conceptually thinks of stdout/stdin being separately locked then it's possible to get into a deadlock when mixing another resource's lock.

For these reasons I'm going to close this for now, but I would encourage you to write up a discuss post if you want to pursue this to gauge the general opinion of others to see if this is a desirable change and what the possible impact could be. Regardless, though, thanks again for the PR!