ids1024
commented
1 year ago Perhaps methods should take T: AsFd instead of BorrowedFd. Given BorrowedFd: AsFd anyway.
Closed ids1024 closed 1 year ago
ids1024
commented
1 year ago Perhaps methods should take T: AsFd instead of BorrowedFd. Given BorrowedFd: AsFd anyway.
notgull
commented
1 year ago 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:
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.Poller? This doesn't really work for 'static polling mechanisms.ids1024
commented
1 year ago 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
commented
1 year ago 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.
codecov[bot]
commented
1 year ago Base: 89.52% // Head: 89.28% // Decreases project coverage by -0.24% :warning:
Coverage data is based on head (
1cee2d9) compared to base (df59c4a). Patch coverage: 87.62% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
notgull
commented
1 year ago 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
commented
1 year ago 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.
Operating on
RawFds is unsafe, so this should be changed. Though some uses will be cleaner oncenixis updated for IO safety (which seems to be ongoing, but complicated).The
Rc<RefCell<IoDispatcher>>inAsyncis somewhat awkward here. For nowDispatcherstill contains aRawFd. The only alternative I can think of with how the API works is to useRc<F>and haveinto_innercallRc::try_unwrap(self.fd).unwrap().