Replace FdGuard with OwnedFd #234
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. Events and Watches now have a lifetime referencing the original file descriptor, since they have a BorrowedFd referencing it.
WhyNotHugo
changed the title
There was a problem hiding this comment.
Hey @WhyNotHugo, thank you again for the new pull request!
I had a quick look, left some comments. Not sure where to go from here, to be honest. Maybe we can have a BorrowedFd for the regular event, but use an Arc<OwnedFd> for EventOwned? Seems a bit more complex than I'd like 🤷
| @@ -18,15 +16,15 @@ use crate::watches::WatchDescriptor; | |||
| /// [`Inotify::read_events_blocking`]: crate::Inotify::read_events_blocking | |||
| /// [`Inotify::read_events`]: crate::Inotify::read_events | |||
| #[derive(Debug)] | |||
| pub struct Events<'a> { | |||
| fd: Weak<FdGuard>, | |||
| pub struct Events<'a, 'i> { | |||
There was a problem hiding this comment.
Adding a second lifetime seems redundant with the one that's already there. Aren't they both the same lifetime, in the end?
There was a problem hiding this comment.
One lifetime references the buffer, the other references the fd. They're not the same, but it's likely that we can use just one.
There was a problem hiding this comment.
Ah, and what I was missing, is that those two things only live together in EventStream, but not in Inotify. Sorry for the noise.
| @@ -188,7 +186,7 @@ impl<'a> Event<&'a OsStr> { | |||
| } | |||
|
|
|||
| /// An owned version of `Event` | |||
| pub type EventOwned = Event<OsString>; | |||
| pub type EventOwned<'i> = Event<'i, OsString>; | |||
There was a problem hiding this comment.
Ah, so we already have an owned event, like I was suggesting in the other issue 😄
By adding a lifetime to it, it's no longer owned though, which defeats the purpose of having it in the first place. That's also the source of your problem with the stream, I'd say. I guess whoever wrote this originally had the same problem, and introduced EventOwned to solve it (a solution that you're undoing here).
I'm not sure exactly what to do. Maybe make Event and WatchDescriptor generic over the kind file descriptor type used, just like Event is already generic over the string type?
There was a problem hiding this comment.
Our options here are:
- Remove the
fdfromEventOwned. - Keep only a
raw_fdinEventOwned.
We can't implement |
Yeah, that's a good point. I'm trying to help, but I've noticed multiple times now, the limited time that I have available for this project isn't enough to think through this problem. Sorry, I wish I could do more, but it is what it is. I'll be here to review whatever you come up with. But I think it'll fall to you (or any other contributor who's interested; it's not like this is your problem any more than it is mine 😄) to come up with a solution, and explain that to me well enough, that I feel confident pressing "Merge" in the end. |
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.
Events and Watches now have a lifetime referencing the original file descriptor, since they have a BorrowedFd referencing it.
Pending issues
A
Streamimplementation cannot return items referencing itself. TheStreamcurrently owns the buffer, so items have a lifetime referencing the streamStream. I'm not yet sure how to resolve this.This is not a problem for the
Iteratorimplementation because the iterator (Inotify) does not own the buffer.