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

Support FinalizationRegistry in Hermes #1440

Open
cyrillicw opened this issue Jun 19, 2024 · 8 comments
Open

Support FinalizationRegistry in Hermes #1440

cyrillicw opened this issue Jun 19, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@cyrillicw
Copy link

Problem

Is FinalizationRegistry support planned to be added to Hermes? It would be convenient to have finalizers for managing native resources in React Native, and is supported by most of modern JS engines

@cyrillicw cyrillicw added the enhancement New feature or request label Jun 19, 2024
@tmikov
Copy link
Contributor

tmikov commented Jun 19, 2024

It is blocked on React Native implementing an event loop that calls drainMicrotasks(). Similarly, Hermes has had an implementation of WeakRef for years, but to this day it can't be used for the same reason.

The RN folks are working on a new event loop. Once that lands and become standards, we can implement FinalizationRegistry.

@cyrillicw
Copy link
Author

cyrillicw commented Jun 20, 2024

@tmikov Thank you for your answer! According to this issue facebook/react-native#42742 they are going to support the new event loop and WeakRefs in the next React Native version 0.75. Can we expect that after that FinalizationRegistry might be implemented?

jhugman added a commit to jhugman/uniffi-bindgen-react-native that referenced this issue Jul 31, 2024
Fixes #10 .

According to [The Big O of Code
Reviews](https://www.egorand.dev/the-big-o-of-code-reviews/), this is a
O(_n_) change.

This PR adds C++ destructor hooks from uniffi `interface` objects, to
call the corresponding Rust `drop` methods.

The impedance mismatch between a GC'd language and an
[RAII](https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization)
language means that one is a leaky abstraction for the other.

From
facebook/hermes#982 (comment):

> Generally speaking, when dealing with native resources, it is
recommended to free the native resource explicitly when you are done
with it, because garbage collection is unpredictable and should not be
relied on for freeing native resources timely."

From the HostObject's destructor docs:

> The C++ object's dtor will be called when the GC finalizes this
object.  (This may be as late as when the Runtime is shut down.)
You have no control over which thread it is called on.  This will
be called from inside the GC…

For this reason, the `uniffiDestroy` method is left in place for now: if
you absolutely need to drop a Rust peer object, then you should call the
`uniffiDestroy` method of the correspond JS object.

From the integration tests alone, it is difficult to show that this is
working; they are too short-lived for the Runtime to be still running
and garbage collection to have taken place.

However, the `test_callbacks.ts` did cause an ungraceful shutdown when
the destructor called into Rust which then called back into JS– which
had already been killed.

The situation will only really get better when React Native/Hermes
[supports a robust `FinalizationRegistry`
implementation](facebook/hermes#1440).
@cray0000
Copy link

Thankfully FinalizationRegistry can be simulated using WeakRef.

Here is an implementation which is working on RN 0.75 on New Architecture:

https://gist.github.com/cray0000/abecb1ca71fd28a1d8efff2be9e0f6c5

It tries to cleanup stuff every 10 seconds.

@tmikov
Copy link
Contributor

tmikov commented Sep 20, 2024

@cray0000 your implementation seems good to me, though obviously it wouldn't scale very well with many objects. One minor nit: what happens if finalize() throws an exception?

FWIW, now that the new architecture will be the default, we are discussing implementing FinalizationRegistry natively.

@cray0000
Copy link

@tmikov good point.

I did initially wrap its call into setTimeout(0) as a hack to just ignore any sync errors during finalize. But then it became harder to debug user-space code when there are multiple finalize`s which have a similar error in them and they all fire roughly at the same time.

And I didn't research the actual FinalizationRegistry standard to understand what the correct behavior is in such situations so I simplified the control flow and just did let it crash synchronously.

But if you think that adding the FinalizationRegistry to Hermes and having RN actually turn it on will take a long time (half a year or more?) then I guess it makes sense to publish this as a library and make it a full-featured mock implementation which follows the standard.

I guess my main idea is that it's finally possible to implement an easy to use cache structures in the standard RN and Expo projects. And I just threw this in for the fellow state management libraries authors as a reference implementation - that the main thing which we needed is already there - it's the WeakRef. And the FinalizationRegistry does not actually matter that much - the destructor logic can be ran by simply checking if the WeakRef is alive.

Having a native implementation is waaaay better of course. It would be a huge win for the RN libraries ecosystem if you could make it happen. Please 🙏

@tmikov
Copy link
Contributor

tmikov commented Sep 20, 2024

@cray0000 I wonder whether as a stopgap we could ship this implementation as a built-in polyfill?

@cray0000
Copy link

@tmikov well, there is no point in asking me, I'd definitely say YES since I heavily depend on having an easy to use destructors for caching :)

And I remember from reading the WeakRef issue on RN repo that Apollo and/or Prisma also really wanted the WeakRef/FinalizationRegistry for their caching system too.

I think that even if it's a user-space implementation, sub-optimal from the performance perspective, having it available is still better than not having it available. I'd guess that for the library authors having a consistent APIs across web and RN and no memory leaks is probably a good trade to make for some performance drop compared to an ideal native implementation.

@tmikov
Copy link
Contributor

tmikov commented Sep 20, 2024

Cool! I think we will do that as the first step. Meanwhile we will work on a "proper" native implementation. Which one gets into the next release is a matter of timing, but at least one certainly will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants