Replace FdGuard with OwnedFd #233
Conversation
An OwnedFd provides the same guarantees: that the underlying file descriptor is closed only once. The file descriptor is behind an Arc, so it will be closed once the last copy of the Arc is dropped.
| } | ||
| } | ||
| } | ||
|
|
||
| impl IntoRawFd for Inotify { | ||
| #[inline] | ||
| fn into_raw_fd(self) -> RawFd { | ||
| self.fd.should_not_close(); | ||
| self.fd.fd | ||
| self.fd.as_raw_fd() |
There was a problem hiding this comment.
This can be problematic; the RawFd can outlive the OwnedFd past the point where it has been closed.
We can't convert the OwnedFd into a RawFd, because it's behind an Arc, so we don't actually own it here.
In practice, actually converting the RawFd into anything is unsafe, so we're not breaking any rules or leaving room for any undefined behaviour.
This is somewhat of an API change, given that the previous implementation leaked the file descriptor instead, and some consumers MAY rely on this (although technically, that would be unsound and depending on an undocumented implementation detail).
There was a problem hiding this comment.
I don't think this can actually work. Most likely, self.fd is the only copy of the Arc, which means OwnedFd will be dropped right here, and the file descriptor will be closed. This would render this whole IntoRawFd implementation non-functional.
I think the only way this could work, is if the Arc has been cloned. Which, if I'm not missing anything, only happens if somebody called watches and is keeping that Watches instance around indefinitely.
This ties into the discussion in #227. And I think it's a sign that the current weird way of doing it is just wrong 😄
I think we should change Watches to have an fd: &OwnedFd instead (which would require a lifetime on the whole struct). Then we can get rid of the Arc and call into_raw_fd here.
There was a problem hiding this comment.
This would render this whole IntoRawFd implementation non-functional.
This returns a RawFd. Converting that into anything "usable" (via FromRawFd) is always unsafe, because the consumer has no way of ensuring that the Fd hasn't been closed.
I think that eventually, Inotify should implement std::os::fd::AsFd, which uses lifetimes to ensure that the fd is not closed.
There was a problem hiding this comment.
Let's try to address #227 first anyway.
There was a problem hiding this comment.
The lack of guarantees by RawFd is a meaningless technicality in this context.
IntoRawFd has a specific purpose, to transfer ownership of the file descriptor. Right now, it fulfills that purpose. With the change proposed here, it would close the file descriptor on transfer (unless the user is doing something weird and specific with another part of the API), rendering the transfer meaningless. And most likely breaking any working applications that currently use this API as intended.
In that case, the IntoRawFd implementation might as well not exist. In fact, that would be the preferable option.
There was a problem hiding this comment.
You're right. I was thinking of AsRawFd, but this is IntoRawFd.
There was a problem hiding this comment.
Thank you for this pull request, @WhyNotHugo! It's really nice to be able to get rid of this home-grown piece of redundant infrastructure.
Unfortunately, I think this is a non-starter as-is, due to the problem in IntoRawFd. See my comment here. I'd love to merge this pull request (or one like it) eventually, but I think we need to fix the situation around Watches first.
| /// Closes the inotify instance | ||
| /// | ||
| /// Closes the file descriptor referring to the inotify instance. The user | ||
| /// usually doesn't have to call this function, as the underlying inotify | ||
| /// instance is closed automatically, when [`Inotify`] is dropped. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Directly returns the error from the call to [`close`], without adding any | ||
| /// error conditions of its own. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use inotify::Inotify; | ||
| /// | ||
| /// let mut inotify = Inotify::init() | ||
| /// .expect("Failed to initialize an inotify instance"); | ||
| /// | ||
| /// inotify.close() | ||
| /// .expect("Failed to close inotify instance"); | ||
| /// ``` | ||
| /// | ||
| /// [`close`]: libc::close | ||
| pub fn close(self) -> io::Result<()> { | ||
| // `self` will be dropped when this method returns. If this is the only | ||
| // owner of `fd`, the `Arc` will also be dropped. The `Drop` | ||
| // implementation for `FdGuard` will attempt to close the file descriptor | ||
| // again, unless this flag here is cleared. | ||
| self.fd.should_not_close(); | ||
|
|
||
| match unsafe { ffi::close(**self.fd) } { | ||
| 0 => Ok(()), | ||
| _ => Err(io::Error::last_os_error()), | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd rather keep this method as a no-op. The documentation should change accordingly.
I'd be fine with deprecating it, as it's superfluous. But I'd like to keep the breaking changes to a minimum
| } | ||
| } | ||
| } | ||
|
|
||
| impl IntoRawFd for Inotify { | ||
| #[inline] | ||
| fn into_raw_fd(self) -> RawFd { | ||
| self.fd.should_not_close(); | ||
| self.fd.fd | ||
| self.fd.as_raw_fd() |
There was a problem hiding this comment.
I don't think this can actually work. Most likely, self.fd is the only copy of the Arc, which means OwnedFd will be dropped right here, and the file descriptor will be closed. This would render this whole IntoRawFd implementation non-functional.
I think the only way this could work, is if the Arc has been cloned. Which, if I'm not missing anything, only happens if somebody called watches and is keeping that Watches instance around indefinitely.
This ties into the discussion in #227. And I think it's a sign that the current weird way of doing it is just wrong 😄
I think we should change Watches to have an fd: &OwnedFd instead (which would require a lifetime on the whole struct). Then we can get rid of the Arc and call into_raw_fd here.
An OwnedFd provides the same guarantees: that the underlying file descriptor is closed only once. The file descriptor is behind an Arc, so it will be closed once the last copy of the Arc is dropped.