Finalizer-like feature #32

MatteoArella · 2020-09-27T16:19:30Z

This PR would close #5.

The finalization is achieved with PhantomReference support since java.lang.ref.Cleaner is introduced only with Java 9.

Now each NativeObject is associated with a NativeObjectReference which contains all necessary data for closing the native object after it becomes phantom reachable.

For letting Context objects to not being garbage collected before Sender or Receiver are closed first, a reference to the context object is added to sender and receiver NativeObjectReference objects.

gavv · 2020-09-28T07:12:27Z

Great! I'll take a look in upcoming days.

src/main/java/org/rocstreaming/roctoolkit/AutoCloseThread.java

src/main/java/org/rocstreaming/roctoolkit/ReferenceCollector.java

…typos

ortex · 2020-10-10T11:39:37Z

LGTM!

gavv

I've started reviewing the PR, here are two preliminary comments.

gavv · 2020-10-12T11:34:48Z

src/main/java/org/rocstreaming/roctoolkit/AutoCloseThread.java

+        while (isRunning()) {
+            try {
+                NativeObjectReference reference = (NativeObjectReference) referenceQueue.remove(MAX_REMOVE_TIMEOUT_MS);
+                synchronized (this) {
+                    remove(reference);
+                    reference.close();
+                }
+            } catch (Exception e) {
+            }
+        }


So basically we're adding a thread that will wake up every 50 ms, right?

I think this is a very bad idea from performance point of view. Especially on battery-powered devices.

Do you think we really need to stop this thread at all? To me, the main advantage of auto-close feature is reducing leaks in long-running applications. Closing objects before shutdown is probably not that important. Or maybe there are cases when the VM is shut down, but the process is not terminated and C heap lives longer than JVM?

If in your opinion stopping the thread is important / useful / nice, I don't mind, but in this case can we use blocking remove() (without timeout) + Thread.interrupt(), to avoid frequent wake-ups?

I've supposed that all roc objects must be freed explicitly before shutdown; according to what you said that's not the case, so we can just set AutoCloseThread as daemon thread, use blocking remove() and get rid of shutdown cleanup.
When the JVM is stopped all process memory (including C heap) is freed and reclaimed by the OS.

I can imagine two drawbacks of NOT closing Roc objects at JVM shutdown:

You have created JVM from C program, run it for a while, then terminated JVM and continued running C program. You'll get a leak in C heap. Even if this is possible, this looks like a very rare case.

When Roc will implement RTCP, RTSP, and other control protocols, proper closing may perform correct termination, e.g. send BYE or TERADOWN, while just terminating JVM will only close the socket. However the other side anyway should be ready to such situation becasue remote peer can just crash without proper closing.

So it seems that proper closing theoretically may be better in some situations, but is likely not critical. Given that the user can always use AutoCloseable or just call close manually, I think that's not a big deal.

Thus, up to me, if there is an easy way to implement this without hurting performance and complicating code, we can do it. Otherwise, let's use daemon thread.

used daemon thread in #40

gavv · 2020-10-12T12:23:48Z

src/main/java/org/rocstreaming/roctoolkit/Receiver.java

+     * @throws IllegalArgumentException     if the arguments are invalid.
+     * @throws Exception                    if an error occured when creating the receiver.
+     */
+    private static long validate(Context context, ReceiverConfig config) throws IllegalArgumentException, Exception {


Maybe better name these methods validateAndOpen() or tryOpen() or something like this? It was surprising to me that validates calls open.

Sure I'll rename these to tryOpen.

renamed in #40

gavv

I've finished the review:

gavv · 2020-10-16T09:01:33Z

src/main/java/org/rocstreaming/roctoolkit/NativeObject.java

+        thread.start();
+
+        /* add a ShutdownHook for closing all NativeObjects that are still open */
+        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+            thread.closeAll();
+            try {
+                thread.join(MAX_JOIN_TIMEOUT_MS);
+            } catch (InterruptedException e) {}
+        }));


Maybe better hide this thread creation/termination in AutoCloseThread.getInstance()? I think this code belongs to AutoCloseThread.

Sure!
Anyway according to official Java documentation all registered shutdown hooks will run all concurrently and in unspecified order so there could be a problem if for example a user adds its own shutdown hook which makes use of roc objects inside it.
In my mind there could be three solutions for that:

Add a note on the README documenting explicitely to not use any roc object inside a user defined shutdown hook

Remove the addShutDownHook mechanism and expose a public method for closing all objects (I like this solution less than all the others because we are potentially giving this responsability to a lazy user which will not use any closing mechanism during program execution and closing objects only at the very end)

Remove the addShutDownHook mechanism at all

Interesting! Hm, given that this PR is quite big already, I would suggest the third approach - remove the addShutDownHook mechanism at all. At least for now.

If the user calls close(), there is no leaks. If the user forgets to call close(), there is no strict guarantee. Bindings will collect dangling objects in some typical use cases, but the correct way to use bindings is still to call close().

I think this is a good approach to start with.

removed shutdownHook in #40

src/main/java/org/rocstreaming/roctoolkit/NativeObject.java

gavv · 2020-10-16T09:10:44Z

src/main/java/org/rocstreaming/roctoolkit/NativeObjectReference.java

+    public void close() throws Exception {
+        if (isOpen()) {
+            this.isOpen.set(false);
+            destructor.close(ptr);
+            this.dependsOn.set(null);
+        }
+    }


We need thread-safe / re-entrant close(), right?

This implementation seems to be not re-entrant. Concurrent close() calls may call destructor twice. To make it re-entrant, we can use atomic CAS operation instead of isOpen() + osOpen.set(false), or use mutex.

Absolutely true! I'll change it with a CAS operation.

fixed in #40

gavv · 2020-10-16T09:27:14Z

src/main/java/org/rocstreaming/roctoolkit/NativeObject.java

+    /**
+     *  Underlying roc object native pointer.
+     */
+    private final long ptr;
+
+    /**
+     *  Destructor method.
+     */
+    private final Destructor destructor;


Duplicating fields in NativeObject and NativeObjectReference looks a bit confusing to me.

Currently the flow is the following:

user creates object derived from NativeObject

NativeObject initializes its fields (ptr and destructor)

NativeObject calls AutoCloseThread

AutoCloseThread creates NativeObjectReference

NativeObjectReference requests fields from NativeObject and copies them

AutoCloseThread returns NativeObjectReference

NativeObject saves NativeObjectReference

NativeObject calls NativeObjectReference.close(), which uses those saved fields

It was quite hard to me to investigate this sequence.

What do you think about the following change?

ptr and destructor fields will present only in NativeObjectReference

NativeObject constructor will create NativeObjectReference (by itself, without help of AutoCloseThread) and pass ptr and destructor to its constructor

then NativeObject constructor will register created NativeObjectReference to AutoCloseThread

NativeObject.getPtr() will call NativeObjectReference.getPtr()

I think this way the relationship between NativeObject and NativeObjectReference will be more clear. NativeObjectReference will hold the raw pointer, its destructor, and will implement closing. NativeObject will be a stateless wrapper on top of it, which just contains a reference to NativeObjectReference and registers and de-registers it in AutoCloseThread.

Also, maybe rename NativeObject to something else, to avoid confusion with NativeObjectReference? NativeObjectReference looks like it's a reference to NativeObject, but reality is actually the opposite. Maybe rename NativeObject to something like BridgedObject?

Actually the NativeObjectReference needs a ReferenceQueue to be passed to its constructor and it's responsibility of the AutoCloseThread to manage the ReferenceQueue; so in my opinion it's better to keep AutoCloseThread to create a NativeObjectReference for better loose coupling.
Surely we can remove the duplicate ptr and destructor fields from NativeObject and let only NativeObjectReference to store them.

The new flow will be:

NativeObject constructor calls AutoCloseThread.add passing ptr and destructor to it

AutoCloseThread creates the NativeObjectReference (which stores the ptr and the destructor) and returns it

NativeObject saves NativeObjectReference

NativeObject.getPtr() will call NativeObjectReference.getPtr()

Also, maybe rename NativeObject to something else, to avoid confusion with NativeObjectReference? NativeObjectReference looks like it's a reference to NativeObject, but reality is actually the opposite. Maybe rename NativeObject to something like BridgedObject?

Sure I'll rename it to BridgedObject

Sounds good!

gavv · 2020-10-16T09:30:47Z

src/main/java/org/rocstreaming/roctoolkit/NativeObjectReference.java

+/**
+ * <code>NativeObjectReference</code> is associated with a {@link NativeObject} and owns its entire lifetime;
+ *
+ * A <code>NativeObjectReference</code> contains necessary data for closing the native object
+ * after it becomes phantom reachable.
+ */
+class NativeObjectReference extends PhantomReference<NativeObject> implements AutoCloseable {


Could you clarify why we need AutoCloseable here?

Here it is just to explicitly expose a close method by that interface.

gavv · 2020-10-16T09:45:09Z

src/main/java/org/rocstreaming/roctoolkit/ReferenceCollector.java

+        @Override
+        public T next() {
+            if (withDependencies.hasNext()) return withDependencies.next();
+            if (withoutDependencies.hasNext()) return withoutDependencies.next();
+            return null;
+        }


We currently have only two layer of dependencies (receiver/sender on first layer, context on second), but I see that ReferenceCollector is designed as if it supported arbitrary number of layers.

However, will it work correctly if there are more than two layers?

If A depends on B, and B depends on C, then A and B will be both in withDependencies list. Is it guaranteed that A is always before B in this list?

If yes, then could you please add brief comment?

If no, then I'd suggest one of the following:

make the implementation correct even on multi-layer dependencies;

OR: make the limitations of the implementation somehow explicit from the code; e.g. maybe hold to maps (with and without dependencies) and require that the dependency itself can be only in without-dependencies map.

Or, if we decide to remove close-on-shutdown feature, the iterator can be probably removed at all.

With this implementation the iterator doesn't order the withDependencies objects, so in case of multiple levels of depencies like your example it's not guaranteed that A is always before B.
I can do a more general solution simply by storing the references in a directed graph and then the iterator ordering will be a result of a topological sort on the graph.

added to sender/receiver link to context. So context wouldn't be collected by CG before sender/reciever. With such approach, we don't need a directed graph. Fixed in #40

gavv · 2022-11-27T17:01:59Z

@MatteoArella Hi, do you have any plans on this?

ortex · 2022-12-02T20:40:18Z

Hi @MatteoArella !
I continue your work in #40. Feel free to code review 😄

gavv · 2022-12-05T21:40:52Z

We've merged #40!

gavv · 2022-12-05T21:41:10Z

@ortex @MatteoArella thank you both

phantom references finalizer

eacbfb5

MatteoArella requested review from gavv and ortex September 27, 2020 16:19

ortex reviewed Oct 3, 2020

View reviewed changes

use referenceQueue.remove() instead of referenceQueue.poll() and fix …

6c2885c

…typos

gavv reviewed Oct 12, 2020

View reviewed changes

gavv reviewed Oct 16, 2020

View reviewed changes

ortex mentioned this pull request Dec 3, 2022

Feature/finalizers #40

Merged

gavv closed this Dec 5, 2022

Uh oh!

Finalizer-like feature #32

Finalizer-like feature #32

Uh oh!

Conversation

MatteoArella commented Sep 27, 2020

Uh oh!

gavv commented Sep 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ortex commented Oct 10, 2020

Uh oh!

gavv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavv Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavv commented Nov 27, 2022

Uh oh!

ortex commented Dec 2, 2022

Uh oh!

gavv commented Dec 5, 2022

Uh oh!

gavv commented Dec 5, 2022

gavv left a comment •

edited

Loading

gavv Oct 16, 2020 •

edited

Loading