Partial implementation of `close` for stdin/out/err (#40032) #46063

zackw · 2017-11-17T19:19:46Z

I'm in the middle of a Happy Fun Air Travel Experience™ and needed to kill a couple hours, so here is an incomplete implementation of feature request #40032 (providing a way to close the standard I/O streams).

This is only the low-level code required for Unix. I know how to write the low-level code required for Redox and Windows, but first I need a little guidance through the maze of structs and traits in between the low-level sys/*/stdio.rs and application code. Currently this will fail to compile with errors like

error: method is never used: `close`
  --> src/libstd/sys/unix/stdio.rs:43:5
   |
43 |     pub fn close() -> io::Result<()> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I'm pretty sure that's because there are no wrapper functions at higher levels, but I'm really not sure where to put them.

This is only the low-level code required for Unix.

cuviper · 2017-11-17T23:48:38Z

src/libstd/sys/unix/stdio.rs

+        // by `fd.replace` is dropped and thus closed.
+        fd.replace(open_null_device(true)?)?;
+        // Don't close STDIN_FILENO itself, though!
+        fd.into_raw();


Errors in those prior ? will drop fd without calling your into_raw. Perhaps you could wrap it in ManuallyDrop instead?

Oh, good point. Thanks for the tip!

kennytm · 2017-11-22T11:11:02Z

Hi @zackw, do you intend to proceed completing this PR?

zackw · 2017-11-22T14:35:12Z

@kennytm If I can get the advice I asked for in the PR cover letter, yes.

carols10cents · 2017-11-27T14:54:29Z

I know how to write the low-level code required for Redox and Windows, but first I need a little guidance through the maze of structs and traits in between the low-level sys/*/stdio.rs and application code.

Ping @rust-lang/libs, can someone help out with this please?

alexcrichton · 2017-11-27T18:23:59Z

src/libstd/sys/unix/fd.rs

+    /// _overall_ operation is not atomic; concurrent threads that snoop on
+    /// the set of valid file descriptors (which they shouldn't) can observe
+    /// intermediate states.
+    pub fn replace(&mut self, other: FileDesc) -> io::Result<FileDesc> {


I don't think the replacement semantics are actually needed when closing, right? That is, I think it's safe for this to return () and avoid the first call to duplicate for the specific use case of Stdout::close?

Yes, I realized the next morning that any caller that actually needs to preserve the old file can just call duplicate itself. And that will dovetail better with the semantics of SetStdHandle on Windows. I will make this change.

alexcrichton · 2017-11-27T18:24:18Z

src/libstd/sys/unix/stdio.rs


 pub struct Stdin(());
 pub struct Stdout(());
 pub struct Stderr(());

+// FIXME: This duplicates code from process_common.rs.


Ah yeah, would it be possible to extract the two to common code?

Yeah, where should it live? File::open_bit_bucket maybe?

alexcrichton · 2017-11-27T18:25:33Z

@zackw the organization is typically just the same set of functions on each platform, the "maze" is typically what's in libstd at src/libstd/io/stdio.rs whereas the definitions in src/libstd/sys/*/stdio.rs basically just need to look the same from an API point of view.

zackw · 2017-11-27T19:00:16Z

the "maze" is typically what's in libstd at src/libstd/io/stdio.rs

I'm sorry, could you please be more specific? I already read through that file and I was not enlightened.

alexcrichton · 2017-11-28T17:46:34Z

@zackw hm I think I may just misunderstand the question or the need for information here. What's the end goal here?

zackw · 2017-11-28T18:05:22Z

@alexcrichton It is clear to me that close methods need to be added to the impl Std{in,out,err} blocks in libstd/io/stdio.rs (because there is no trait to hang them off of) and that these methods ultimately need to forward to concrete implementations in libstd/sys/*/stdio.rs. What I don't understand is whether and to what extent the Std{in,out,err}Raw and Std{in,out,err}Lock classes need to participate in this forwarding, because there is a lot of almost-but-not-quite parallel code in their impls and it's confusing.

I also can't figure out if there is any other place I should be looking at for other classes that also need to be involved.

alexcrichton · 2017-11-29T00:15:08Z

Ah ok, I can try to help out with that! So in general this is touching on the question of methods like File::close as well, and the libs team currently hasn't reached a conclusion/consensus on how we'd like to do "close related methods". An issue for this is rust-lang/api-guidelines#61

In that sense it's not even clear how we would add close-related methods to the standard library (API-wise). Something like Stdout::close() is a possibility but there's also io::stdout().close() for example. Now the method here will be unstable so we can always finish the debate later for the signature, so it's not absolutely critical that consensus is achieved right now.

I would personally recommend adding something like:

impl Stdout {
    pub fn close(&mut self) -> io::Result<()> {
        // ...
    }
}

That would then probably delegate to a similar method on the versions in the sys modules. It's also good to track and record these issues and possible design decisions in the tracking issue as it will help inform the stabilization of these methods.

zackw · 2017-11-29T01:08:02Z

Thanks, that's helpful in terms of the larger picture, but it's almost entirely unrelated to the question I was trying to ask. What I'm stuck on is not the appropriate signature of Stdout::close, but whether the helper types StdoutRaw and StdoutLock need to grow close methods as well. I have just about persuaded myself that StdoutLock should, because we certainly want to serialize this operation against writes in another thread, but I don't properly understand what StdoutRaw is even for...

alexcrichton · 2017-11-29T01:10:28Z

That's sort of also a question that is not easily answered unfortunately. We can review more during stabilization but the most conservative route is to likely just add it to Stdout and such. I would avoid the locked instances to be conservative and the "raw" instances as they're unstable anyway.

* FileDesc::replace() no longer returns the old file; call duplicate() yourself if you need it. * Make sure STD{IN,OUT,ERR}_FILENO don't get closed on errors, using ManuallyDrop. * Correct signatures of close methods.

Close methods are available on both Std{in,out,err} and Std{in,out,err}Lock, because closing these files should be serialized against reads/writes in other threads. They are *not* available on Std{in,out,err}Raw because that layer of indirection seems to be to do with the set_print/set_panic mechanism, which *doesn't* operate by changing what fds 0/1/2 refer to. That maybe ought to get cleaned up, but Not In This Patchset™. The test is known to succeed on Unix.

Very nearly the same as the code for Unix. (What's with the extra buffer argument to dup2, though?) Note: the duplicate code for opening the null device should get refactored, but we need to decide where to put it first, and that probably needs its own feature discussion.

zackw · 2017-12-02T18:06:45Z

With the update, I believe I have addressed all of the review comments on the low-level Unix code. I have also added a low-level implementation for Redox, high-level wrappers in io/stdio.rs, and a test (in test/run-pass/).

Still missing is a low-level implementation for Windows. It turns out that I don't understand Windows stream/console handles and how Rust works with them well enough to write that part, after all.

retep998 · 2017-12-02T20:30:42Z

So on Windows there is no equivalent to dup2. There is no way to change an existing handle to be something else. Instead the only way to change what stdout/stdin/stderr are is to use SetStdHandle which has some interesting consequences. The first is that it does not return the old value, so there is no way to atomically exchange the current stdout/stdin/stderr, meaning you could get in a race condition with another thread trying to change stdout/stdin/stderr. The second is that some thread might still be using the old value, and if you close the handle, it is now effectively a dangling handle. The dangerous aspect is the handle could be reused in the future, and code still holding on to the old value will now start reading/writing to something completely different. If you don't close the handle, then you're leaking handles.

zackw · 2017-12-04T00:17:42Z

@retep998

SetStdHandle ... does not return the old value

We don't actually need that, though, I think? The original iteration of the Unix code had replace returning the old file descriptor, but that got taken out almost immediately.

some thread might still be using the old value, and if you close the handle, it is now effectively a dangling handle. The dangerous aspect is the handle could be reused in the future, and code still holding on to the old value will now start reading/writing to something completely different. If you don't close the handle, then you're leaking handles.

I'm afraid this makes no sense at all to me, I think because I don't know which handle you mean by "this handle". So let's be very concrete here: What is wrong with this more-or-less direct translation of the Unix operations to Win32?

    let hNullDev = CreateFileW(L"NUL:", GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, 0, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, 0);
    if (hNullDev == INVALID_HANDLE_VALUE) return Err(GetLastError());
    if (SetStdHandle(STD_OUTPUT_HANDLE, hNullDev)) {
        int e = GetLastError();
        CloseHandle(hNullDev);
        return Err(e);
    }
    CloseHandle(hNullDev);
    return Ok;

Specifically, I am guessing at two things that you could have meant, both of which seem extremely unlikely to me, but my intuitions are not tuned for Windows.

This doesn't actually close the old stdout; you have to do something else as well (what?)
It's not safe to close hNullDev on success; it has to be kept around until the program exits and/or the next time someone calls SetStdHandle.

Please clarify?

retep998 · 2017-12-04T02:34:35Z

You claim that this PR is an implementation of #40032 which is specifically about closing stdout so the parent process can be notified that it was closed. However this cannot be done sanely!

Code can, and often does, cache the result of GetStdHandle. This means that after calling SetStdHandle code may still be using the old value. Even without caching, there is always at least a short duration of time between calling GetStdHandle and writing to the handle.
When you close a handle with CloseHandle, there is no guarantee that another handle won't be created with the same numerical identifier.
The combination of the above two points means if you call SetStdHandle and then try to CloseHandle the old handle, code which has the old value cached could end up writing to a completely different random handle with disastrous consequences. It's akin to writing to a dangling pointer.

alexcrichton · 2017-12-04T15:09:36Z

@zackw functionality like this may need to be a unix-specific extension trait rather than a cross-platform implementation, I agree that the desired semantics here I believe cannot be implemented safely by us on Windows.

zackw · 2017-12-04T15:36:19Z

@retep998 I'm sorry, I still don't get it.

Code can, and often does, cache the result of GetStdHandle

So what? GetStdHandle(STD_OUTPUT_HANDLE) does a DuplicateHandle internally, same as if someone calls dup(1) on Unix, doesn't it? A subsequent SetStdHandle operation will change the open file backing STD_OUTPUT_HANDLE, but there's another live handle, so the original file won't actually be closed. Perfectly fine.

(Everything else you said seems to be predicated on the assumption that this is wrong, so let's focus on getting this piece sorted out first.)

alexcrichton · 2017-12-04T17:14:15Z

@zackw you can sort of think of it in terms of ownership I think. Something like dup(1) returns an owned value, which is sort of like calling Rc::clone. The GetStdHandle function, however, is like returning a reference (not an owned value), so while someone is using GetStdHandle you can't change it. (AFAIK it doesn't "dup" the return value and you're not supposed to CloseHandle the return value)

retep998 · 2017-12-04T19:23:03Z

@zackw GetStdHandle does not do a DuplicateHandle internally. It merely returns the value of the internal global variable. When you call SetStdHandle the previous value is simply forgotten, and not closed. It really just acts as a way to get and set three pointer sized values in a process global way.

carols10cents · 2017-12-12T14:51:40Z

@zackw friendly triage ping! How's this going?

zackw · 2017-12-12T15:41:50Z

@carols10cents I don't have a lot of time to work on this right now and I'm not sure when this will change. @retep998 has convinced me that this is not something that can be reasonably implemented on Windows, and that means the methods have to be moved to some kind of unix/redox-specific extension trait (unless having them throw unimplemented! on Windows would be acceptable) and I'm worried that that drags in the unresolved design questions from rust-lang/api-guidelines#61.

If someone with more time wants to take over the work I won't stand in the way.

carols10cents · 2017-12-12T17:24:57Z

Ok, so should we close this pr?

…

On Dec 12, 2017 9:42 AM, "Zack Weinberg" ***@***.***> wrote: @carols10cents <https://github.com/carols10cents> I don't have a lot of time to work on this right now and I'm not sure when this will change. @retep998 <https://github.com/retep998> has convinced me that this is not something that can be reasonably implemented on Windows, and that means the methods have to be moved to some kind of unix/redox-specific extension trait (unless having them throw unimplemented! on Windows would be acceptable) and I'm worried that that drags in the unresolved design questions from rust-lang/api-guidelines#61 <https://github.com/rust-lang-nursery/api-guidelines/issues/61>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46063 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAL1UmQaZJ_7reNX5hq-JKYmRUOq_z7_ks5s_p7rgaJpZM4QigGo> .

alexcrichton · 2018-01-04T16:02:47Z

It's been a few weeks now so I'm going to close this for now, but we can always reconsider a resubmission!

Partial implementation of close for stdin/out/err (rust-lang#40032)

b645bc9

This is only the low-level code required for Unix.

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 17, 2017

cuviper reviewed Nov 17, 2017

View reviewed changes

kennytm added E-needs-mentor T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 22, 2017

alexcrichton reviewed Nov 27, 2017

View reviewed changes

alexcrichton self-assigned this Nov 27, 2017

kennytm removed the E-needs-mentor label Nov 27, 2017

zackw added 3 commits December 2, 2017 12:50

Address review comments on low-level Unix code.

1188c1a

* FileDesc::replace() no longer returns the old file; call duplicate() yourself if you need it. * Make sure STD{IN,OUT,ERR}_FILENO don't get closed on errors, using ManuallyDrop. * Correct signatures of close methods.

Add feature annotations to doctests.

8e137e5

alexcrichton closed this Jan 4, 2018

kennytm added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial implementation of `close` for stdin/out/err (#40032) #46063

Partial implementation of `close` for stdin/out/err (#40032) #46063

zackw commented Nov 17, 2017 •

edited

Loading

cuviper Nov 17, 2017

zackw Nov 27, 2017

kennytm commented Nov 22, 2017

zackw commented Nov 22, 2017

carols10cents commented Nov 27, 2017

alexcrichton Nov 27, 2017

zackw Nov 27, 2017 •

edited

Loading

alexcrichton Nov 27, 2017

zackw Nov 27, 2017

alexcrichton commented Nov 27, 2017

zackw commented Nov 27, 2017

alexcrichton commented Nov 28, 2017

zackw commented Nov 28, 2017

alexcrichton commented Nov 29, 2017

zackw commented Nov 29, 2017

alexcrichton commented Nov 29, 2017

zackw commented Dec 2, 2017

retep998 commented Dec 2, 2017 •

edited

Loading

zackw commented Dec 4, 2017 •

edited

Loading

retep998 commented Dec 4, 2017 •

edited

Loading

alexcrichton commented Dec 4, 2017

zackw commented Dec 4, 2017 •

edited

Loading

alexcrichton commented Dec 4, 2017

retep998 commented Dec 4, 2017 •

edited

Loading

carols10cents commented Dec 12, 2017

zackw commented Dec 12, 2017 •

edited

Loading

carols10cents commented Dec 12, 2017 via email

alexcrichton commented Jan 4, 2018

Partial implementation of close for stdin/out/err (#40032) #46063

Partial implementation of close for stdin/out/err (#40032) #46063

Conversation

zackw commented Nov 17, 2017 • edited Loading

cuviper Nov 17, 2017

Choose a reason for hiding this comment

zackw Nov 27, 2017

Choose a reason for hiding this comment

kennytm commented Nov 22, 2017

zackw commented Nov 22, 2017

carols10cents commented Nov 27, 2017

alexcrichton Nov 27, 2017

Choose a reason for hiding this comment

zackw Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

alexcrichton Nov 27, 2017

Choose a reason for hiding this comment

zackw Nov 27, 2017

Choose a reason for hiding this comment

alexcrichton commented Nov 27, 2017

zackw commented Nov 27, 2017

alexcrichton commented Nov 28, 2017

zackw commented Nov 28, 2017

alexcrichton commented Nov 29, 2017

zackw commented Nov 29, 2017

alexcrichton commented Nov 29, 2017

zackw commented Dec 2, 2017

retep998 commented Dec 2, 2017 • edited Loading

zackw commented Dec 4, 2017 • edited Loading

retep998 commented Dec 4, 2017 • edited Loading

alexcrichton commented Dec 4, 2017

zackw commented Dec 4, 2017 • edited Loading

alexcrichton commented Dec 4, 2017

retep998 commented Dec 4, 2017 • edited Loading

carols10cents commented Dec 12, 2017

zackw commented Dec 12, 2017 • edited Loading

carols10cents commented Dec 12, 2017 via email

alexcrichton commented Jan 4, 2018

Partial implementation of `close` for stdin/out/err (#40032) #46063

Partial implementation of `close` for stdin/out/err (#40032) #46063

zackw commented Nov 17, 2017 •

edited

Loading

zackw Nov 27, 2017 •

edited

Loading

retep998 commented Dec 2, 2017 •

edited

Loading

zackw commented Dec 4, 2017 •

edited

Loading

retep998 commented Dec 4, 2017 •

edited

Loading

zackw commented Dec 4, 2017 •

edited

Loading

retep998 commented Dec 4, 2017 •

edited

Loading

zackw commented Dec 12, 2017 •

edited

Loading