[BUG] Releasing GLFW window callback then replacing it.

[quew8]

Not entirely sure whether this is desired behaviour or not. Strikes me as not since I couldn't find any reference to it in documentation.

Essentially if you set a window callback, release the callback, then replace it with another (as opposed to setting it, replacing it then releasing it), a NullPointerException is thrown.

Example Stack Trace:

Exception in thread "main" java.lang.NullPointerException
   at org.lwjgl.system.MemoryUtil.memGlobalRefToObject(Native Method)
   at org.lwjgl.system.libffi.Closure.create(Closure.java:195)
   at org.lwjgl.glfw.GLFW.glfwSetKeyCallback(GLFW.java:2081)
   at com.quew8.gutils.desktop.windowing.Window.setKeyCallback(Window.java:138)

where setKeyCallback() is this method:

public boolean setKeyCallback(GLFWKeyCallback keyCallback) {
    if(this.keyCallback != null) {
        this.keyCallback.release();
    }
    this.keyCallback = keyCallback;
    return glfwSetKeyCallback(window, keyCallback) != null;
}

whereas this method doesn't produce a NPE:

public boolean setKeyCallback(GLFWKeyCallback keyCallback) {
    this.keyCallback = keyCallback;
    keyCallback = glfwSetKeyCallback(window, keyCallback);
    if(keyCallback != null) {
        keyCallback.release();
        return true;
    } else {
        return false;
    }
}

[spasi]

The GLFW callback functions have been designed to work like this:

GLFWKeyCallback cb = glfwSetKeyCallback(window, newCallbackOrNull);
if ( cb != null )
	cb.release();

If you release the previous callback before doing that, what happens is that the JNI global reference stored in the FFI closure is deleted. That's why you're getting a NPE there. You also risk a segfault, since the FFI closure itself has also been released (probably getting lucky because libFFI uses a custom memory allocator).

In general, it is good practice to release callbacks after they have been "unregistered", since some callbacks may be called asynchronously. In GLFW's case that's not a problem, because callbacks (except Error and Monitor) can only be invoked in glfwPollEvents(), so I can't say that what you're doing is not legitimate.

There are two ways to fix this. a) Document the fact that .release() should only be called after replacing the callback or b) Change the GLFW callback methods to return a raw pointer (long instead of the callback object). This will change the above code to:

long cb = glfwSetKeyCallback(window, null);
if ( cb != NULL )
	GLFWKeyCallback.create(cb).release();

[advanced_set]

I am quite confused. If I understood correctly the design of the new callback system is due the fact that GC may collect java objects leaving callbacks registered in the native code (memory leaks). Quick questions:

1. Having strong references does not defeat the purpose of getting rid of the old Adapter for callbacks? Having a lot of somethingCallback objects as fields seems to be boilerplate code (as usually anonymous classes are used for this).

Note: I am playing in Scala, and for now, the only option is val c = new SomethingCallback { override def invoke (...) }, lambdas and method references seems not be compatible right now.

2. Are the callbacks released when the GLWF windows are destroyed? Do I need to call somethingCallback.release()? When? Before or after destroying the window?

(if they do, why I need to worry about memory leaks?)

[spasi]

I am quite confused. If I understood correctly the design of the new callback system is due the fact that GC may collect java objects leaving callbacks registered in the native code (memory leaks). Quick questions:

1. Having strong references does not defeat the purpose of getting rid of the old Adapter for callbacks? Having a lot of somethingCallback objects as fields seems to be boilerplate code (as usually anonymous classes are used for this).

Please read this post first. Also this one.

Note: I am playing in Scala, and for now, the only option is val c = new SomethingCallback { override def invoke (...) }, lambdas and method references seems not be compatible right now.

The SomethingCallback.SAM interfaces were created for this reason.

2. Are the callbacks released when the GLWF windows are destroyed? Do I need to call somethingCallback.release()? When? Before or after destroying the window?

(if they do, why I need to worry about memory leaks?)

No, there is no automatic cleanup, you need to release callbacks manually.

[advanced_set]

Please read this post first. Also this one.

Thanks! I see now. I read the first one but I had missed the second one. Sorry.

So I guess before destroying a window, we must do something like this:

// pseudocode
somethingCallback cb = somethingCallback(window, null);
if ( cb != null )
    cb.release();

destroy(window)

Even though they are all Retainables, the callbackfunctions are different, so we need to set them null one by one and then release the references.

SAM support in Scala is still experimental:
http://www.scala-lang.org/news/2.12-roadmap

[spasi]

So I guess before destroying a window, we must do something like this:

// pseudocode
somethingCallback cb = somethingCallback(window, null);
if ( cb != null )
    cb.release();

destroy(window)

Yes, but since LWJGL already forces you to hold on to the callbacks with strong references, you can just use those to .release() directly, either before or after destroying the window. This fact makes the returned previous callback instances somewhat useless, so it might be a good idea to remove them* to avoid the issue described by the OP.

* make them just return the function pointer (long) instead of the callback object

[quew8]

Ah I see. I kind of felt like it should be that way. I also feel like since you've got this layer of abstraction around the callbacks, best to keep it up. So a change in the documentation to just mention this is the right way to go about it.

[advanced_set]

Quick question (sorry if I am asking something obvious) :

May I use the same somethingCallback objects (strong references) for several windows and release them only when I destroy the last window?

(provided of course, that they only use local variables / parameters)

[spasi]

Yes, callbacks can safely be reused.

[advanced_set]

that was fast! thanks.