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

std: Stabilize parts of std::os::platform::io #23766

Merged
merged 1 commit into from
Apr 1, 2015

Conversation

alexcrichton
Copy link
Member

This commit stabilizes the platform-specific io modules, specifically around
the traits having to do with the raw representation of each object on each
platform.

Specifically, the following material was stabilized:

  • AsRaw{Fd,Socket,Handle}
  • RawFd (renamed from Fd)
  • RawHandle (renamed from Handle)
  • RawSocket (renamed from Socket)
  • AsRaw{Fd,Socket,Handle} implementations
  • std::os::{unix, windows}::io

The following material was added as #[unstable]:

  • FromRaw{Fd,Socket,Handle}
  • Implementations for various primitives

There are a number of future improvements that are possible to make to this
module, but this should cover a good bit of functionality desired from these
modules for now. Some specific future additions may include:

  • IntoRawXXX traits to consume the raw representation and cancel the
    auto-destructor.
  • Fd, Socket, and Handle abstractions that behave like Rust objects and
    have nice methods for various syscalls.

At this time though, these are considered backwards-compatible extensions and
will not be stabilized at this time.

This commit is a breaking change due to the addition of Raw in from of the
type aliases in each of the platform-specific modules.

[breaking-change]

This commit stabilizes the platform-specific `io` modules, specifically around
the traits having to do with the raw representation of each object on each
platform.

Specifically, the following material was stabilized:

* `AsRaw{Fd,Socket,Handle}`
* `RawFd` (renamed from `Fd`)
* `RawHandle` (renamed from `Handle`)
* `RawSocket` (renamed from `Socket`)
* `AsRaw{Fd,Socket,Handle}` implementations
* `std::os::{unix, windows}::io`

The following material was added as `#[unstable]`:

* `FromRaw{Fd,Socket,Handle}`
* Implementations for various primitives

There are a number of future improvements that are possible to make to this
module, but this should cover a good bit of functionality desired from these
modules for now. Some specific future additions may include:

* `IntoRawXXX` traits to consume the raw representation and cancel the
  auto-destructor.
* `Fd`, `Socket`, and `Handle` abstractions that behave like Rust objects and
  have nice methods for various syscalls.

At this time though, these are considered backwards-compatible extensions and
will not be stabilized at this time.

This commit is a breaking change due to the addition of `Raw` in from of the
type aliases in each of the platform-specific modules.

[breaking-change]
@rust-highfive
Copy link
Collaborator

r? @huonw

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

@alexcrichton
Copy link
Member Author

r? @aturon
cc @carllerche
cc @l0kod

@rust-highfive rust-highfive assigned aturon and unassigned huonw Mar 26, 2015
@carllerche
Copy link
Member

Probably too late, but would this be better as Into<Fd> etc..?

Edit Changed to Into

@blaenk
Copy link
Contributor

blaenk commented Mar 27, 2015

I agree with @carllerche, unless there are reasons for not going that route that I'm not aware of.

Same with From<Fd> etc.

@aturon
Copy link
Member

aturon commented Mar 27, 2015

The problem with using the generic traits is that you lose any notion of "opt in" for platform-specific behavior. With traits defined in the platform modules, it's really clear from a use that the methods you're pulling in will not be available on all platforms.

@carllerche
Copy link
Member

@aturon Would you not get the "opt in" factor by pulling in the target types, like Fd?

@aturon
Copy link
Member

aturon commented Mar 27, 2015

It depends partly on the specifics, but it's possible to write something like file.as_raw() without type constraints and still compile successfully, due to inference of input types on traits.

Regardless, the generic methods are not meant to fully replace named conversion methods; they're intended primarily for generic programming.

@alexcrichton alexcrichton deleted the stabilize-raw-fd branch March 27, 2015 20:43
@alexcrichton alexcrichton restored the stabilize-raw-fd branch March 27, 2015 20:43
@alexcrichton alexcrichton reopened this Mar 27, 2015
@l0kod
Copy link
Contributor

l0kod commented Mar 27, 2015

Nice!

It may be preferable to be the only owner of a RawFd with AsRawFd using a mutable object to avoid concurrent inner file modification. This can be achieve by replacing as_raw_fd(&self) with as_raw_fd(&mut self).

A proper file descriptor ownership passing would save from file descriptor leaking. I prefer a trait-based pattern for FromRawFd: replacing from_raw_fd(fd: RawFd) with from_raw_fd<T>(fd: T) where T: IntoRawFd and return Result<Self>. This allow to safely pass ownership, cleanup the inner object (e.g. stream flushing) and handle error (e.g. fstat the file descriptor) but require an IntoRawFd trait.

The AsRawFd impl should probably take care of buffer flushing as well. The doc should mention this.

Probably the same for *Handle and *Socket.

@alexcrichton
Copy link
Member Author

It may be preferable to be the only owner of a RawFd with AsRawFd using a mutable object to avoid concurrent inner file modification. This can be achieve by replacing as_raw_fd(&self) with as_raw_fd(&mut self).

I fear that this may be overkill for this low-level method unfortunately. It seems like an extra "safety barrier" which may end up backfiring when you're in a situation like trying to get a file descriptor out of an Arc<TcpStream>, for example. I'm also not sure how much benefit you really get by expressing via &mut self because I can always squirrel away the file descriptor elsewhere and then mutate it when I no longer own it (e.g. there's no lifetime of the returned value).

I prefer a trait-based pattern for FromRawFd: replacing from_raw_fd(fd: RawFd) with from_raw_fd(fd: T) where T: IntoRawFd and return Result.

I fear that this is also perhaps too generalized for this use case. The trait is intended to represent the minimal step of construction from a file descriptor to an I/O object. It is in all cases currently just a noop, which is quite a powerful guarantee. The trait is intended to be an extension trait as well, not necessarily a general purpose trait. Along these lines I don't think we want to implement a conversion from something like BufRead<TcpStream> to File just because the representation happens to be a file descriptor underneath.

I think any form of fallible conversion would also be done through a separate extension trait to clearly document why it may fail and be clear about what's going on.

@l0kod
Copy link
Contributor

l0kod commented Mar 28, 2015

Replacing from_raw_fd(fd: RawFd) with from_raw_fd<T>(fd: T) where T: AsRawFd (instead of IntoRawFd) seems reasonable.

It leave the possibility to create your own FileDesc/Fd implementing AsRawFd and handling the file descriptor closing with it's own Drop (if you need to create objects from arbitrary/FFI RawFd values). This also remove the need for a Return<Self> for FromRawFd but indirectly move the error handling to AsRawFd implementations' constructor which is cleaner (and already done).

@l0kod
Copy link
Contributor

l0kod commented Mar 28, 2015

from_raw_fd<T>(fd: &mut T) where T: AsRawFd seems as simple, doesn't take ownership but is safer.

@l0kod
Copy link
Contributor

l0kod commented Mar 28, 2015

I'm also not sure how much benefit you really get by expressing via &mut self because I can always squirrel away the file descriptor elsewhere and then mutate it when I no longer own it (e.g. there's no lifetime of the returned value).

Yes, you can, but only if you really want to. We should avoid RawFd from floating around as much as possible but use AsRawFd as long as possible.

@alexcrichton
Copy link
Member Author

from_raw_fd<T>(fd: &mut T) where T: AsRawFd seems as simple, doesn't take ownership but is safer.

Note that one of the primary points of these apis is that it does take ownership. In general we're not trying to create a one-size-fits-all "create this object from a file descriptor" abstraction, and erring on the side of being conservative is generally the better option for the standard library. Along these lines I think there's a few downsides to the two signatures you've proposed:

  • from_foo(T) where T: AsFoo is somewhat against our constructor conventions where from_foo normally consumes foo
  • We don't necessarily want the ability to say TcpStream::from_raw_fd(my_file_desc_structure), but instead explicitly relinquishing ownership of the file descriptor via my_file_desc_structure.into_raw_fd(), for example.
  • from_foo(&mut T) where T: AsFoo is also against our conventions where it takes &mut T instead of &T.

In general these APIs are the conservative option here as they are not designed to enable generic programming over AsRawFd, for example. These are purely extension traits which are providing new constructors/accessors for types on unix platforms.

@aturon
Copy link
Member

aturon commented Mar 30, 2015

I think we should move forward with this PR. While I agree with @l0kod that we will want to pursue safer abstractions in the long run, this lowest-level API will always have a place. This is part of the reason for shifting to "RawFd" naming everywhere -- we want to leave plenty of room to add a higher-level notion of Fd later on, if we desire. I think it's quite analogous to raw pointers.

@bors: r+ 6370f29

@bors
Copy link
Contributor

bors commented Mar 30, 2015

⌛ Testing commit 6370f29 with merge 0a9618d...

@bors
Copy link
Contributor

bors commented Mar 31, 2015

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member Author

@bors: retry

On Mon, Mar 30, 2015 at 5:13 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/4274


Reply to this email directly or view it on GitHub
#23766 (comment).

@bors
Copy link
Contributor

bors commented Mar 31, 2015

⌛ Testing commit 6370f29 with merge fce17ab...

@bors
Copy link
Contributor

bors commented Mar 31, 2015

💔 Test failed - auto-mac-64-nopt-t

@alexcrichton
Copy link
Member Author

@bors: retry

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 31, 2015
This commit stabilizes the platform-specific `io` modules, specifically around
the traits having to do with the raw representation of each object on each
platform.

Specifically, the following material was stabilized:

* `AsRaw{Fd,Socket,Handle}`
* `RawFd` (renamed from `Fd`)
* `RawHandle` (renamed from `Handle`)
* `RawSocket` (renamed from `Socket`)
* `AsRaw{Fd,Socket,Handle}` implementations
* `std::os::{unix, windows}::io`

The following material was added as `#[unstable]`:

* `FromRaw{Fd,Socket,Handle}`
* Implementations for various primitives

There are a number of future improvements that are possible to make to this
module, but this should cover a good bit of functionality desired from these
modules for now. Some specific future additions may include:

* `IntoRawXXX` traits to consume the raw representation and cancel the
  auto-destructor.
* `Fd`, `Socket`, and `Handle` abstractions that behave like Rust objects and
  have nice methods for various syscalls.

At this time though, these are considered backwards-compatible extensions and
will not be stabilized at this time.

This commit is a breaking change due to the addition of `Raw` in from of the
type aliases in each of the platform-specific modules.

[breaking-change]
@bors bors merged commit 6370f29 into rust-lang:master Apr 1, 2015
@alexcrichton alexcrichton deleted the stabilize-raw-fd branch April 2, 2015 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants