FD: remove big surrounding RefCell, simplify socketpair #3809
Conversation
| } | ||
|
|
||
| // Check if the peer_fd closed | ||
| if self.peer_fd.upgrade().is_none() { |
There was a problem hiding this comment.
Okay looking at the old code here I think I have a theory for why behavior changed. But I can't imagine that the old behavior was intentional.
In think previously we had a situation where writebuf still existed but the peer was already destroyed. That's because a file descriptor reference gets invalidated before the call to FileDescriptor::close, but the Rc inside of it (in this case, for the buffer) survive until they get dropped, which happens at the end of FileDescriptor::close.
|
|
FD: remove big surrounding RefCell, simplify socketpair A while ago, I added the big implicit RefCell for all file descriptions since it avoided interior mutability in `eventfd`. However, this requires us to hold the RefCell "lock" around the entire invocation of the `read`/`write` methods on an FD, which is not great. For instance, if an FD wants to update epoll notifications from inside its `read`/`write`, it is very crucial that the notification check does not end up accessing the FD itself. Such cycles, however, occur naturally: - eventfd wants to update notifications for itself - socketfd wants to update notifications on its "peer", which will in turn check *its* peer to see whether that buffer is empty -- and my peer's peer is myself. This then also lets us simplify socketpair, which currently holds a weak reference to its peer *and* a weak reference to the peer's buffer -- that was previously needed precisely to avoid this issue.
|
Sorry, I tweaked the comment a bit more.^^ @bors r=oli-obk |
|
☀️ Test successful - checks-actions |
| // Write to fd[0]. | ||
| let data = "abcde".as_bytes().as_ptr(); | ||
| res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5).try_into().unwrap() }; | ||
| let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; |
There was a problem hiding this comment.
@tiif btw, the test becomes a lot more readable if you avoid let mut res and all the try_into().unwrap(). Just use a new fresh res variable each time. :)
4 participants
A while ago, I added the big implicit RefCell for all file descriptions since it avoided interior mutability in
eventfd. However, this requires us to hold the RefCell "lock" around the entire invocation of theread/writemethods on an FD, which is not great. For instance, if an FD wants to update epoll notifications from inside itsread/write, it is very crucial that the notification check does not end up accessing the FD itself. Such cycles, however, occur naturally:This then also lets us simplify socketpair, which currently holds a weak reference to its peer and a weak reference to the peer's buffer -- that was previously needed precisely to avoid this issue.