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

Use io-lifetimes types in API, instead of RawFd #119

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Dec 16, 2022

Operating on RawFds is unsafe, so this should be changed. Though some uses will be cleaner once nix is updated for IO safety (which seems to be ongoing, but complicated).

The Rc<RefCell<IoDispatcher>> in Async is somewhat awkward here. For now Dispatcher still contains a RawFd. The only alternative I can think of with how the API works is to use Rc<F> and have into_inner call Rc::try_unwrap(self.fd).unwrap().

@ids1024
Copy link
Member Author

ids1024 commented Dec 16, 2022

Perhaps methods should take T: AsFd instead of BorrowedFd. Given BorrowedFd: AsFd anyway.

@notgull
Copy link
Member

notgull commented Dec 17, 2022

This is a bit hard to pull off successfully, with full I/O safety. For instance, let's say that a source is dropped before it's deregistered from the Poller. This is undefined behavior, especially if the Poller is currently waiting.

But then, you have a handful of options:

  • Do we pass ownership of the source to the Poller, by converting it into OwnedFd? While this could work, it ends up being somewhat clumsy, especially if you need to access the source during a wait.
  • Do we borrow the source for the lifetime of the Poller? This doesn't really work for 'static polling mechanisms.

@ids1024
Copy link
Member Author

ids1024 commented Dec 18, 2022

Strictly speaking I don't think that's undefined behavior, since calloop (at least with the backends implemented) doesn't actually keep the file descriptor. Epoll and kqueue maintain an association with the file description in-kernel.

(If calloop implemented a backend for platforms that only support poll/select, this would be a concern. Presumably IOCP would be similar to epoll/kqueue in this context, but I'm not familiar with it.)

Epoll(7) apparently will still deliver events after closing the fd if there are still fd's open to the same file description (if you dup an fd you get a new "file descriptor" referencing the same "file description"). Kqueue(2) says close will remove any kevents with that fd, but then it also says, "events which are attached to file descriptors are automatically deleted on the last close of the descriptor" (what is the "last close" if it's talking about descriptors?).

So I think the poller shouldn't cause UB as long as the fd is valid at the time it is passed to one of the methods. Is there any case where this could lead to unexpected/undesired (but not "undefined") behavior, or inconsistencies between epoll and kqueue?

@elinorbgr
Copy link
Member

Thanks LGTM overall. The CI error is not a fluke though, the calloop book needs to be adjusted for these changes, and this needs a changelog entry, as this is a breaking change (as illustrated by the book CI failure).

Regarding safety, in particular:

For instance, let's say that a source is dropped before it's deregistered from the Poller. This is undefined behavior, especially if the Poller is currently waiting.

First, unless your EventSource implementation does not own its underlying FD (which is dubious design), it's not possible for it to be dropped while the poller is waiting, as the EventLoop is not threadsafe.

Second, my understanding is the save as @ids1024: closing an FD without deregistering from the poller would at most cause spurious wakeups of the event loop, no unsafety.

ids1024 added a commit to ids1024/wayland-rs that referenced this pull request Dec 19, 2022
Allows the `Display` to be used in `calloop::generic::Generic`, in
combination with Smithay/calloop#119.
Operating on `RawFd`s is unsafe, so this should be changed. Though some
uses will be cleaner once `nix` is updated for IO safety (which seems
to be ongoing, but complicated).

The `Rc<RefCell<IoDispatcher>>` in `Async` is somewhat awkward here. For
now `Dispatcher` still contains a `RawFd`. The only alternative I can
think of with how the API works is to use `Rc<F>` and have `into_inner`
call `Rc::try_unwrap(self.fd).unwrap()`.
Adds an unsafe helper for wrapping an `AsRawFd` implementer in `AsFd`.
Which is needed with things like zmq until they update to use IO safety
traits: erickt/rust-zmq#361.
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Base: 89.52% // Head: 89.28% // Decreases project coverage by -0.24% ⚠️

Coverage data is based on head (1cee2d9) compared to base (df59c4a).
Patch coverage: 87.62% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
- Coverage   89.52%   89.28%   -0.25%     
==========================================
  Files          17       17              
  Lines        1805     1838      +33     
==========================================
+ Hits         1616     1641      +25     
- Misses        189      197       +8     
Flag Coverage Δ
macos-latest 88.15% <67.18%> (-0.97%) ⬇️
ubuntu-latest 87.41% <80.72%> (-0.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sources/ping.rs 100.00% <ø> (+14.28%) ⬆️
src/sys/mod.rs 89.31% <72.41%> (-3.13%) ⬇️
src/io.rs 82.65% <80.00%> (ø)
src/sources/generic.rs 78.87% <80.00%> (-5.88%) ⬇️
src/loop_logic.rs 92.18% <100.00%> (+0.31%) ⬆️
src/sources/ping/eventfd.rs 92.40% <100.00%> (+0.19%) ⬆️
src/sources/ping/pipe.rs 98.52% <100.00%> (+1.42%) ⬆️
src/sources/signals.rs 80.23% <100.00%> (ø)
src/sys/epoll.rs 98.33% <100.00%> (+0.18%) ⬆️
src/sys/kqueue.rs 98.89% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@elinorbgr elinorbgr merged commit cba4d3a into Smithay:master Jan 9, 2023
@notgull
Copy link
Member

notgull commented Jan 9, 2023

Sorry for not saying this earlier, but maybe you could have it take Into<OwnedFd> instead of AsFd, and just have it own the source?

@ids1024
Copy link
Member Author

ids1024 commented Jan 9, 2023

As the API currently works, making register take an OwnedFd couldn't work, because then we'd transfer ownership and wouldn't be able to call unregister for the same fd. You also want to be able to use the type implementing AsFd in the callback.

What we might need to safely use things like poll is to invert the API in some way, so the EventSource has some method returning a BorrowedFd (perhaps depend on an AsFd implementation) that could be polled as long as the source exists.

@ids1024 ids1024 deleted the io-lifetimes branch January 19, 2023 00:09
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.

3 participants