Add File::try_clone

[sfackler]

I have it set as stable right now under the rationale that it's extending an existing, stable API to another type in the "obvious" way.

r? @alexcrichton

cc @reem

[reem]

Looks great; looking forward to removing my much more fragile platform specific code.

[alexcrichton]

This seems reasonable to me, although I will cc @rust-lang/libs and tag with it to make sure we're all good with the insta-stablilization.

[oli-obk]

I know this can't cause memory unsafety, but isn't it very unexpected to be able to modify a file from two threads simultaneously (without additional synchronization, writing to the file will be very amusing at best)

[alexcrichton]

@oli-obk I suspect so, yes, but there are uses now and then which are nice if the standard library supports them. That being said I can't particularly think of a concrete reason to have this, but perhaps @sfackler or @reem had one in mind to motivate this?

[sfackler]

@oli-obk I hate to be the bearer of unexpected news, but that is already the case: Read and Write are implemented for &File, and all of the state mutation methods on it take &self. This just makes it more ergonomic to work with.

@alexcrichton I implemented this at @reem's request, so he'd be able to better speak to the use cases.

[oli-obk]

Ugh, ugly. Still, there you are at least bound to an instance. So what's the difference between Rc<File> right now and File in this PR, except that the "sharedness" is hidden?

[sfackler]

It's pretty annoying to access the impls for &File. Calling foo.read(buf) where foo is an Arc<File> will try to select the implementation for File and fail to compile: http://is.gd/RnmzQZ. Since the impl is for &File and not Arc<File>, you can't use it in contexts that want a 'static Reader.

[reem]

Hmm, I actually could get by in my case with an Arc<File> (I have a case where I would normally option-dance around the File, but I would rather avoid keeping the File in an Option for all the other cases), but why implement my own reference counting on top of the underlying os's reference count?

EDIT: For clarity, the case is moving from one enum variant which contains a File to another enum variant which should contain the same File with an &mut T where T is the enum.

[oli-obk]

I don't know how powerful impl specialization would be, but could it create a special impl for Rc<File> and Arc<File> that uses the filehandle reference counting instead moving the File object to the heap?

[retep998]

@oli-obk That seems overly complicated and would be a lot of complexity when simply duplicating a file handle can be done now fairly easily with just a single public method.

[oli-obk]

I'm aware of that. But it's much clearer that Rc<File> or Arc<File> are shared and File is owned. Without using hypothetical impl specialization it would still work although with a little overhead.

---

Since the borrowing semantics of File are broken already, we might just go all the way. Then we'd also need a way to obtain the reference count so we can figure out if we truly own the handle. Also some more documentation about this deviation from ownership semantics would be good.

[retep998]

Even if you duplicate a File both Files still own their handles, the handles just happen to have some shared state. Also, as far as I can tell there is no way to get the reference count of a handle on Windows.

[oli-obk]

Yes, but shared ownership is represented through Rc or Arc in Rust. At least I don't know of any other shared ownership types.

Also, as far as I can tell there is no way to get the reference count of a handle on Windows.

It's probably not important anyway. We can always get true ownership back through a newtype that doesn't give access to the internal value. Still a sad deviation, but one that's already happened, now we gotta live with it.

[alexcrichton]

@oli-obk I at least would consider us to not perform that sort of specialization or optimization in the future. There are some use cases where you want an entirely separate handle and/or file descriptor to pass to a library which it itself calls close on, for example. That means that there's a semantic difference between an OS-reference-counted File and Rc<File>. The former case has whole new file descriptors allocate where the latter all shares the same file descriptor.

[bluss]

insta-stabilizing seems wrong (why is this PR special)? If any time is right for it, it's this week though; it will still need 11 weeks to reach the stable release.

[sfackler]

Fair enough, I'm fine with landing it unstable. I'll update it tonight.

[sfackler]

Updated to be unstable.

[aturon]

I'm fine with this functionality, under the rubric of exposing cross-platform IO APIs that apply to our various IO objects (part of the vision of IO reform).

The name isn't great, though. If you succeed here, you haven't really cloned, you've created a new alias. (And we also avoid try for Result-returning operations). How about duplicate_handle? Verbose, but I don't think this will be called very often in Rust.

[alexcrichton]

@aturon note, however, that we have already stabilized TcpStream::try_clone, so I believe this is just mirroring that

[BurntSushi]

@aturon To bikeshed a bit more, clone on Arc/Rc also means "created a new alias," so there is some precedent!

[aturon]

@BurntSushi

Yeah, I thought about that, though I'd argue it's mildly more obvious what's going on in those cases.

@alexcrichton

D'oh! I'm surprised that got through, though I imagine I signed off on it at some point :-)

Given the try_clone precedent, we should probably stick with the name here. Not worth the effort of renaming the stable method, even if it's not great.

[sfackler]

Agreed that try_clone is not a great name; I filed #31239 to track renaming in the future.

[alexcrichton]

The libs team discussed this during triage today and the conclusion was to merge (mirrors existing APIs, seems like good functionality to have).

@sfackler could you open a tracking issue and update the pointer here? Other than that r=me

[sfackler]

@bors r=alexcrichton

[bors]

📌 Commit a414b61 has been approved by alexcrichton

[bors]

⌛ Testing commit a414b61 with merge 7b9d6d3...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

[oconnor663]

Asking 3.5 years after the fact: Does anyone know why we set inheritable = true here? I've run into a bug (because duct was abusing File::try_clone somewhat) where pipes get inherited by processes that aren't supposed to inherit them, when spawning happens on multiple threads at once. Normally the CREATE_PROCESS_LOCK prevents this, but this line here creates inheritable handles outside of that lock. duct is going to work around this by making all the appropriate system calls explicitly. But this inheritable flag might be cause some programs in the wild to keep files open longer than they should (even without a multi-threaded race condition like mine), and on Windows in particular that could cause bugs by making them un-deletable.