Skip to content
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

Amend RFC 517: Add material for stdio #899

Merged
merged 6 commits into from
Mar 13, 2015
Merged

Conversation

alexcrichton
Copy link
Member

Expand the section on stdin, stdout, and stderr while also adding a new section
explaining the fate of the current print-related functions.

Rendered

Expand the section on stdin, stdout, and stderr while also adding a new section
explaining the fate of the current print-related functions.
@@ -72,7 +73,6 @@ follow-up PRs against this RFC.
* [TCP]
* [UDP]
* [Addresses]
* [std::net] (stub)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to delete this when the std::net RFC landed.

@l0kod
Copy link

l0kod commented Feb 24, 2015

The format!/write!/… macros should write only once, not call write(2) multiple times.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 24, 2015

Then they have to allocate memory first which is not acceptable.

@l0kod
Copy link

l0kod commented Feb 24, 2015

Maybe a new macro for that purpose then (e.g. formatbf!)?

@mahkoh
Copy link
Contributor

mahkoh commented Feb 24, 2015

So just write!(dst, "{}", format!(whatever))

@l0kod
Copy link

l0kod commented Feb 24, 2015

Yeah, not sure we want to write this for each println!

@aturon
Copy link
Member

aturon commented Feb 24, 2015

I think the move we made in old_io to providing a global stdin buffer, together with locking, has worked pretty well (thanks @sfackler!)

Personally, I would suggest the following variant of the design for consistency:

  • stdin as you proposed it,
  • stdout and stderr as also using a global lock (and, for stdout, global buffering), in particular so that conveniences like write_all behave atomically. Alternatively, we could skip the lock for stderr since it's unbuffered, if atomicity for convenience methods is not considered important enough.
  • _raw variants for all three (no locking or buffering) in std::io

Just to clarify why this kind of global design is useful, consider some alternatives:

  • We could just not buffer at all (basically, just expose the raw variants), but that would be a surprise to most people and would not be very ergonomic. If you failed to buffer, you'd pay in performance, and if you did buffer, you'd have to set it up yourself (probably including some kind of global to share the buffer between different functions). Centralizing this functionality with good defaults is worthwhile in my opinion.
  • We could forgo the atomicity but retain semi-global buffering by allocating buffers in a thread-local way (dropping the locks). By buffering thread-locally, we would (1) ensure that buffering happens and (2) avoid problems like stdin().read_line(..); stdin().read_line() creating multiple buffers. But the stdin handle would be effectively unusable across threads, since even if e.g. those thread we're coordinating when to read lines, data would be locked away in one thread's buffer when another tried to read.

On the other hand, of course it's important to provide all of the raw handles to that people can build their own buffering/coordination strategy without paying any cost.

I don't think the RFC says where these constructors should live, but I would propose std::io for all of them (including the raw variants).

Also add back text for `foo_raw` and details.
@alexcrichton
Copy link
Member Author

@aturon

Personally, I would suggest the following variant of the design for consistency:

I like the sound of all this. I've pushed an update which includes:

  • All global handles are locked by default.
  • The stdout handle is now globally buffered
  • Raw versions of each handle have been added as foo_raw for each respective version.
  • APIs and trait implementations sketched out.

@l0kod

The format!/write!/… macros should write only once, not call write(2) multiple times.

As @mahkoh mentioned these are fundamentally unable to prevent many calls to the Write::write function without some form of intermediate buffering. It is likely that objects used with write! will need to use some form of buffering to ensure reasonable performance. This is why, for example, the stdout stream is buffered-by-default. Today each call to println! is likely to only invoke write(2) once.

@aturon
Copy link
Member

aturon commented Feb 25, 2015

BTW, I strongly suggest we land some preliminary version of this ASAP, since we need to start getting feedback on the new APIs. We can and should be very clear that none of it will be stabilized until the RFC is finalized, of course.

@alexcrichton
Copy link
Member Author

I have created a PR for this RFC, but I would like to stress that it is not final at all. We need to implement these APIs to allow crates to migrate to std::io which facilitates the rapid implementation, but it's perfectly ok for the RFC to diverge from the implementation and all changes will be implemented promptly upon acceptance of this RFC.

@l0kod
Copy link

l0kod commented Feb 25, 2015

Don't forget the FromRawFd trait ;)

@l0kod
Copy link

l0kod commented Feb 25, 2015

This API looks really like a custom implementation of a generic file-descriptor/handle type (e.g. IoHandle/IoDesc).
It would be nice to have an inner (common) file descriptor type with the lock feature (cf. Lazy<Mutex<T>>).

@alexcrichton
Copy link
Member Author

@l0kod

Don't forget the FromRawFd trait ;)

We're currently leaving that for a future RFC as it's only somewhat tangentially related to the stdio bits of this RFC.

This API looks really like a custom implementation of a generic file-descriptor/handle type

I agree! We're just not ready at this time to commit to that kind of an API.

@alexcrichton
Copy link
Member Author

I've pushed an update which is pretty different in how the raw input/output handles are dealt with, especially on windows. It also clarifies what happens on windows for the stdout/stdin handles.

cc @nagisa, @retep998

@mahkoh
Copy link
Contributor

mahkoh commented Feb 25, 2015

More #ifdef hell for code that wants to be portable. At this point I would be content with stdout_raw and friends simply returning io::Result<StdoutRaw> and friends on all platforms.

@alexcrichton alexcrichton self-assigned this Feb 26, 2015
@mahkoh
Copy link
Contributor

mahkoh commented Feb 27, 2015

The set_stdout and set_stderr functions will be moved to a new
std::fmt::output module and renamed to set_print and set_panic,
respectively. These new names reflect what they actually do, removing a
longstanding confusion. The current stdio::flush function will also move to
this module and be renamed to flush_print.

set_stderr should be called set_error_print and should be accessible via println_err and print_err macros or similar. Furthermore these Writers should be stored in Mutexes and be inherited by child threads. It's currently not possible to capture the full output of a thread because the thread can itself spawn child threads which don't inherit the stdout and stderr. This can be seen when one uses multithreaded tests.

@alexcrichton
Copy link
Member Author

I've pushed an update which recommends that the raw stdio primitives are not implemented or stabilized at this time. Otherwise the interface remains the same and continues to be somewhat high level. It's quite similar to what's implemented today, but a few tweaks will be necessary for the implementation.

The update also revises that std::fmt::output will not be added at all, it will be removed with no replacement.


@mahkoh I agree there are some interesting questions around std::fmt::output and I've removed it from the RFC.

```

* `stdin` - returns a handle to a **globally shared** standard input of
the process which is buffered as well. All operations on this handle will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it seems this 'all' isn't quite true? (Or is it referring to the internals too.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? (I may be missing something)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are operations other than lock on Stdin, meaning a user can do things with it without locking. (I'm now inclined to believe I just misread: and this sentence is saying those operations would just call lock internally.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, yes this is just saying that all methods will call lock internally

@huonw
Copy link
Member

huonw commented Mar 5, 2015

Hm, the windows behaviour seems to be very "fancy" to me, less minimal than we might hope for std. However, it seems the problem itself is sufficiently complicated that it may not be possible to get a totally minimal resolution while maintaining some semblance of sanity? (I do not know enough about IO on windows to judge with much precision.)

@alexcrichton
Copy link
Member Author

Hm, the windows behaviour seems to be very "fancy" to me, less minimal than we might hope for std. However, it seems the problem itself is sufficiently complicated that it may not be possible to get a totally minimal resolution while maintaining some semblance of sanity.

I agree, I'm not super comfortable about this either. I've been considering it a hard constraint though that the stdio handles must implement Read and Write, and given that I think that this is basically the most reasonable thing to do on windows for now. I personally also think that giving access to the raw handles when possible will also help quite a bit.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 8, 2015

This still doesn't address writing to stderr. Having to create your own thread local variable just to do some error reporting is annoying.

@alexcrichton
Copy link
Member Author

@mahkoh

This still doesn't address writing to stderr. Having to create your own thread local variable just to do some error reporting is annoying.

Can you clarify where this thread local variable is coming from? The new primitives are not thread local, and the std::fmt::output module is not being added and the old functionality is being removed.

@nikomatsakis
Copy link
Contributor

@alexcrichton one thing I found confusing in the current text was the discussion of locking. It wasn't clear to me whether the value returned by stdin itself performed locking, or whether that was the responsibility of the client. In particular this text feels ambiguous:

All operations on this handle will first require acquiring a lock to ensure access to the shared buffer is synchronized. The handle can be explicitly locked for a critical section so relocking is not necessary. The Read trait will be implemented directly on the returned Stdin handle but the BufRead trait will not be (due to synchronization concerns). The locked version of Stdin will provide an implementation of BufRead.

It may be that this text is implicitly referencing the existing API or other things with which I am unfamiliar? In general though this text sounds to me as if the locking is something the user does ("first require acquiring a lock", "relocking is not necessary", "ButRef trait will not be impemented"), but there is no coverage of what API would be offered. I am guessing that I am misinterpreting this paragraph though.

@alexcrichton
Copy link
Member Author

@nikomatsakis I tried to clarify that section a bit to make it a little more explicit, but let me know if anything is off!

aturon added a commit that referenced this pull request Mar 13, 2015
Amend RFC 517: Add material for stdio
@aturon aturon merged commit 3857a25 into rust-lang:master Mar 13, 2015
@aturon
Copy link
Member

aturon commented Mar 13, 2015

This RFC has now been merged. It is a conservative refinement of the previous incarnation of stdio handles, in particular clarifying the interaction with Windows consoles/unicode requirements.

We will definitely need to revisit this topic to introduce raw handles and redirection for println and panic, but that can all be added backwards-compatibly.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 13, 2015

but that can all be added backwards-compatibly.

No it cannot. The RFC says that stdout() writes to the stdout of the process, that is, it's equivalent to writing to fd 1. Code that needs this guarantee would have to be changed if at any point stdout() can become thread specific.

@aturon
Copy link
Member

aturon commented Mar 13, 2015

@mahkoh

I'm not referring to changing the target of stdout; I'm talking about the target of println and panic. In principle code could be assuming that these macros target fd 1, but I expect their contract to not actually make such a promise given the intent to allow redirection later.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 13, 2015

What are you thinking that you'd consider having println write to something other than stdout() a good design? How can println not just being a shortcut for writeln!(stdout(), ...).unwrap() be acceptable?

@nagisa
Copy link
Member

nagisa commented Mar 13, 2015

Stdout/in/err are unlikely to ever become thread local again. We used that design in old_io and it didn’t work out. Now we have one a single handle and are locking for thread safety instead.

Regarding println! redirection: we already have writeln! macro, which is basically a println! with an argument to specify output writer.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 13, 2015

@nagisa: How did it not work out? Why do you think that that implies that it cannot work out?

Regarding println! redirection: we already have writeln! macro, which is basically a println! with an argument to specify output writer

Then libraries that want to allow having their output captured cannot use writeln? And they cannot write anything to stderr? That's ridiculous.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 13, 2015

For example, it is a serious problem in some terminal programs that you cannot capture the output of libraries which then mess up your output. You have to hack around this by replacing fd 1 and 2 temporarily with /dev/null before library calls (e.g.). The rust stdlib should not inherit this problem.

@aturon
Copy link
Member

aturon commented Mar 13, 2015

@mahkoh

What are you thinking that you'd consider having println write to something other than stdout() a good design? How can println not just being a shortcut for writeln!(stdout(), ...).unwrap() be acceptable?

The previous design made it possible to redirect println at the thread level, and in principle that redirection could be inherited -- while still keeping stdout as process level fd 1. Note, this RFC specifically does not take such a step, and I'm just saying that it was a part of the discussion that didn't come to a close here and should be followed up on soon.

For example, it is a serious problem in some terminal programs that you cannot capture the output of libraries which then mess up your output. You have to hack around this by replacing fd 1 and 2 temporarily with /dev/null before library calls (e.g.). The rust stdlib should not inherit this problem.

Right, so I'm wondering what proposal you have in mind? A few of your comments on this thread seem to be alluding to a design, but it'd help to see it spelled out a bit more explicitly.

My main point here is that the current RFC doesn't address these problems and so we need to revisit it. Depending on the precise contract we give, it should be possible to add redirection later, but in any case we can probably address it before 1.0 regardless.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 13, 2015

Right, so I'm wondering what proposal you have in mind?

See #973

@Centril Centril added the A-input-output Proposals relating to std{in, out, err}. label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-input-output Proposals relating to std{in, out, err}.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants