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

ndk: Use BorrowedFd and OwnedFd to clarify ownership transitions #417

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

MarijnS95
Copy link
Member

Depends on #416

Some functions consume a file descriptor (will close them on their own regard) or return ownership over a file descriptor (expect the caller to close it), but this is not always clarified in the documentation nor upheld by the caller. Use the new stabilized BorrowedFd and OwnedFd types since Rust 1.63 to clarify this in the API, noting that OwnedFd will instinctively close() the file descriptor on drop and doesn't implement Copy nor Clone (but does provide a try_clone() helper using fcntl() to create a new owned file descriptor if needed, and if possible by the kernel).

For example, while not obvious from AHardwareBuffer_lock() docs (though there are hints in the graphics sync docs) the source for gralloc buffer locking many function calls down clarifies that the acquireFence is indeed owned and will be closed. The same applies to AImageReader and its async aquire functions.

@MarijnS95 MarijnS95 requested a review from rib August 10, 2023 13:25
@MarijnS95 MarijnS95 added the impact: breaking API/ABI-breaking change label Aug 10, 2023
Base automatically changed from looper-remove-fd to master August 10, 2023 15:28
@MarijnS95 MarijnS95 force-pushed the ownedfd branch 2 times, most recently from a1e3d36 to 3cba71b Compare August 10, 2023 17:45
Comment on lines +55 to +58
/// # Safety
/// The caller should guarantee that this file descriptor remains open after it was added
/// via [`ForeignLooper::add_fd()`] or [`ForeignLooper::add_fd_with_callback()`].
fd: BorrowedFd<'fd>,
Copy link
Member Author

@MarijnS95 MarijnS95 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will think about this a bit more before merging (cc @rib).

This is an fd the user gave to us (as a BorrowedFd, though) so they're already required to make sure it isn't closed until it is removed from the Looper. Second, as written above the unsafe block that creates this:

                // SAFETY: Even though this FD at least shouldn't outlive self, a user could have
                // closed it after calling add_fd or add_fd_with_callback.

There's barely any relevance to self here; the user can totally keep using their own fd outside the scope of Looper, we really have no clue what its lifetime should be.

Most importantly, let's check if winit can easily handle the lifetime that we've now added to Poll<'fd>.

@MarijnS95 MarijnS95 force-pushed the ownedfd branch 2 times, most recently from 080a3b8 to ddff788 Compare August 11, 2023 09:44
@MarijnS95 MarijnS95 marked this pull request as draft August 11, 2023 12:27
@MarijnS95
Copy link
Member Author

Turns out I made the biggest mistake I could here: by calling as_raw_fd() on an OwnedFd we're merely borrowing the OwnedFd, meaning it gets dropped by us at the end of the Rust function rather than "transferring" ownership to the FFI function, meaning close() ends up being called twice.

Converted this PR to draft for that as I want to test out the new codepaths first before merging this... and while setting up a test-case I ran into more lifetime/reference issues in the NDK 😩. Release will probably be bleeding into next week.

Some functions consume a file descriptor (will close them on their own
regard) or return ownership over a file descriptor (expect the caller to
close it), but this is not always clarified in the documentation nor
upheld by the caller.  Use the new stabilized `BorrowedFd` and `OwnedFd`
types since Rust 1.63 to clarify this in the API, noting that `OwnedFd`
will instinctively `close()` the file descriptor on drop and doesn't
implement `Copy` nor `Clone` (but does provide a `try_clone()` helper
using `fcntl()` to create a new owned file descriptor if needed, and if
possible by the kernel).

For example, while not obvious from `AHardwareBuffer_lock()` docs
(though there are hints in the [graphics sync docs]) the source for
gralloc buffer locking many function calls down clarifies that the
[`acquireFence` is indeed owned and will be closed].  The same [applies
to `AImageReader` and its async aquire functions].

[graphics sync docs]: https://source.android.com/docs/core/graphics/sync
[`acquireFence` is indeed owned and will be closed]: https://cs.android.com/android/platform/superproject/main/+/refs/heads/main:frameworks/native/libs/ui/Gralloc4.cpp;l=320-323;drc=34edaadf5297f2c066d2cb09a5cc9366dc35b24b
[applies to `AImageReader` and its async aquire functions]: https://cs.android.com/android/platform/superproject/main/+/refs/heads/main:frameworks/av/media/ndk/NdkImageReader.cpp;l=498-501;drc=34edaadf5297f2c066d2cb09a5cc9366dc35b24b
@MarijnS95 MarijnS95 marked this pull request as ready for review August 15, 2023 23:37
@MarijnS95
Copy link
Member Author

I managed to test the lock() ownership transition with an ImageReader where I use its producer end (the NativeWindow that we refcounted wrongly, #418) in EGL: this lets the ImageReader return a SyncFence (blocking on GL GPU completion) file descriptor when asynchronously acquiring the latest Image, which can subsequently be used to .lock() the HardwareBuffer inside the Image. I tested the ownership transitions here with some libc::close() (since the drop() for OwnedFd discards the error) and confirmed that the NDK closes it for us.

@MarijnS95 MarijnS95 merged commit 5c89cf6 into master Aug 15, 2023
@MarijnS95 MarijnS95 deleted the ownedfd branch August 15, 2023 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: breaking API/ABI-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants