ndk/looper: Add remove_fd() method

[MarijnS95]

We've previously only been able to unregister file descriptors by using a callback and returning false from it to be unregistered. Add the missing method that allows users to unregister their file descriptors from the looper for any reason.

There has always been a "concern" about leaking the Box inside add_fd_with_callback(), but this is already very easy by never returning false from the callback or registering a different callback / event on the same fd, so this new function doesn't really expose that.

[rib]

Looks reasonable to me.

Maybe for the issue with leaking the Box there could be some way of having add_fd_with_callback return some form of handle that could then be used to remove that callback, which could also make sure to drop the Box.

Not sure how that would work with respect to how removeFd doesn't guarantee that there can't be more calls to the callback after removing the fd.

I wonder if it's possible to get better guarantees with addFd(fd, callback=null) to explicitly clear the callback before removeFd

Skimming the implementation I'm not super sure: https://android.googlesource.com/platform/system/core/+/master/libutils/Looper.cpp#432

[MarijnS95]

Maybe for the issue with leaking the Box there could be some way of having add_fd_with_callback return some form of handle that could then be used to remove that callback, which could also make sure to drop the Box.

Perhaps we could, since a ForeignLooper can call remove_fd and is Send/Sync and probably also easily clonable.

Alternatively we could keep a HashMap of sorts mapping fd to Box. Then we can also catch overwrites when the same fd in add_fd(_with_callback)(). Not sure what to do when users close fds and get the same fd back on subsequent open() etc, it's up to them if they forgot to remove_fd it.

---

Anyway I tried to not mention (in the docs) the fact that it might be hard to trigger the callback just to return false currently (maybe via EPOLLHUP) to clean up the Box. But let's mention it here anyway.

[MarijnS95]

Regardless, leaking memory isn't unsafe/UB (these APIs playing with random fds that we have no control over are, maybe they should've been marked unsafe to that effect), so I'll just merge this now I think and revisit this once we have a concrete user with this problem and maybe some more time / interest in thinking up the right solution.

[rib]

Alternatively we could keep a HashMap of sorts mapping fd to Box. Then we can also catch overwrites when the same fd in add_fd(_with_callback)(). Not sure what to do when users close fds and get the same fd back on subsequent open() etc, it's up to them if they forgot to remove_fd it.

One of the risks with this kind of stuff is that technically other non-Rust code, (or potentially even Rust code that has ended up linking in multiple versions of the ndk crate) could end up messing up this kind of state tracking. This is because the underlying loopers are shared via TLS but this state may only be associated with a single ndk looper wrapper.

[MarijnS95]

I intentionally didn't bring that up. Many parts of the ndk are trivial to get raw pointers from (or otherwise) and process "things" outside of our Rust wrapper, and if we have to concern ourselves with that it becomes much harder to build a fully sound wrapper. This is anyway similar to the user e.g. closing the fd while it is still recorded inside the Looper, as mentioned before.