Skip to content

Commit

Permalink
Allocate one single DestroyNotify callback to properly handle scope="…
Browse files Browse the repository at this point in the history
…notified" callbacks. Fixes #125
  • Loading branch information
jwharm committed Sep 22, 2024
1 parent 54a9400 commit c8734ce
Show file tree
Hide file tree
Showing 8 changed files with 131 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public final class ClassNames {
public static final ClassName PROXY_INSTANCE = get(PKG_BASE, "ProxyInstance");
public static final ClassName UNSUPPORTED_PLATFORM_EXCEPTION = get(PKG_BASE, "UnsupportedPlatformException");

public static final ClassName ARENAS = get(PKG_INTEROP, "Arenas");
public static final ClassName ARENA_CLOSE_ACTION = get(PKG_INTEROP, "ArenaCloseAction");
public static final ClassName MEMORY_CLEANER = get(PKG_INTEROP, "MemoryCleaner");
public static final ClassName INTEROP = get(PKG_INTEROP, "Interop");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import java.util.*;

import static io.github.jwharm.javagi.util.Conversions.getValueLayout;
import static io.github.jwharm.javagi.util.Conversions.toJavaIdentifier;
import static java.util.function.Predicate.not;
import static java.util.stream.Collectors.joining;

Expand Down Expand Up @@ -153,11 +152,12 @@ PartialStatement marshalParameters(boolean intAsLong) {
if (!stmt.format().isEmpty()) stmt.add(", ");
stmt.add("$Z"); // emit newline
var generator = new TypedValueGenerator(p);
var name = generator.getName();

// Generate null-check. But don't null-check parameters that are
// hidden from the Java API, or primitive values
if (generator.checkNull())
stmt.add("($memorySegment:T) (" + generator.getName() + " == null ? $memorySegment:T.NULL : ");
stmt.add("($memorySegment:T) (" + name + " == null ? $memorySegment:T.NULL : ");

// cast int parameter to a long
if (intAsLong && p.anyType() instanceof Type t && t.isLong())
Expand All @@ -169,13 +169,21 @@ PartialStatement marshalParameters(boolean intAsLong) {
.filter(q -> q.destroy() == p)
.findAny();
if (notify.isPresent()) {
String notifyName = toJavaIdentifier(notify.get().name());
stmt.add("_" + notifyName + "DestroyNotify.toCallback(_" + notifyName + "Scope)");
stmt.add("$arenas:T.CLOSE_CB_SYM",
"arenas", ClassNames.ARENAS);
} else {
stmt.add("$memorySegment:T.NULL");
}
}

// User_data for destroy_notify
else if (p.isUserDataParameterForDestroyNotify()) {
var cbParam = p.getRelatedCallbackParameter();
var cbName = new TypedValueGenerator(cbParam).getName();
stmt.add("$arenas:T.cacheArena(_" + cbName + "Scope)",
"arenas", ClassNames.ARENAS);
}

// User_data
else if (p.isUserDataParameter())
stmt.add("$memorySegment:T.NULL");
Expand All @@ -190,12 +198,12 @@ else if (p.isOutParameter()
&& type.get() instanceof Alias a
&& a.type().isPrimitive()
&& type.isPointer())) {
stmt.add("_" + generator.getName() + "Pointer");
stmt.add("_" + name + "Pointer");
}

// Custom interop
else
stmt.add(generator.marshalJavaToNative(generator.getName()));
stmt.add(generator.marshalJavaToNative(name));

// Closing parentheses for null-check
if (generator.checkNull())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public String generate(boolean signalDeclaration) {
"@param detail");
writeDoc(builder, "the signal handler",
"@param handler");
writeDoc(builder, "a {@link SignalConnection} object to keep track of the signal connection",
writeDoc(builder, "a signal handler ID to keep track of the signal connection",
"@return");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.squareup.javapoet.MethodSpec;
import io.github.jwharm.javagi.configuration.ClassNames;
import io.github.jwharm.javagi.gir.*;
import io.github.jwharm.javagi.gir.Record;
import io.github.jwharm.javagi.util.PartialStatement;

import java.lang.foreign.Arena;
Expand Down Expand Up @@ -176,9 +175,6 @@ private void scope(MethodSpec.Builder builder) {
if (p.scope() == Scope.NOTIFIED && p.destroy() != null)
builder.addStatement("final $1T _$2LScope = $1T.ofConfined()",
Arena.class,
getName())
.addStatement("final $1T _$2LDestroyNotify = $$ -> _$2LScope.close()",
ClassNames.DESTROY_NOTIFY,
getName());
else if (p.scope() == Scope.ASYNC && (!p.isDestroyNotifyParameter()))
builder.addStatement("final $1T _$2LScope = $1T.ofConfined()",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,7 @@ public TypeSpec generateFunctionalInterface() {
public MethodSpec generateConnectMethod() {
MethodSpec.Builder builder = MethodSpec.methodBuilder(connectMethod)
.addModifiers(Modifier.PUBLIC)
.returns(ParameterizedTypeName.get(
ClassNames.SIGNAL_CONNECTION,
signal.typeName()
));
.returns(int.class);

if (signal.parent() instanceof Interface)
builder.addModifiers(Modifier.DEFAULT);
Expand Down Expand Up @@ -94,13 +91,16 @@ public MethodSpec generateConnectMethod() {
ClassNames.INTEROP,
signal.name());

return builder.addStatement("var _callbackArena = $T.ofConfined()",
return builder.addStatement("var _callbackArena = $T.ofShared()",
Arena.class)
.addStatement("var _callback = handler.toCallback(_callbackArena)")
.addStatement("var _result = (long) $1T.g_signal_connect_data.invokeExact($Zhandle(), _name, _callback, $2T.NULL, $2T.NULL, 0)",
.addStatement("return (int) (long) $1T.g_signal_connect_data.invokeExact("
+ "$Zhandle(),"
+ "$W_name,$Whandler.toCallback(_callbackArena),"
+ "$W$2T.cacheArena(_callbackArena),"
+ "$W$2T.CLOSE_CB_SYM,"
+ "$W0)",
ClassNames.SIGNALS,
MemorySegment.class)
.addStatement("return new SignalConnection<>(handle(), _result, _callbackArena)")
ClassNames.ARENAS)
.nextControlFlow("catch (Throwable _err)")
.addStatement("throw new AssertionError(_err)")
.endControlFlow()
Expand Down
17 changes: 15 additions & 2 deletions buildSrc/src/main/java/io/github/jwharm/javagi/gir/Parameter.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,25 @@ public boolean isUserDataParameter() {
return parent().parameters().stream().anyMatch(p ->
p.anyType() instanceof Type type
&& type.get() instanceof Callback
&& p.closure() == this
);
&& p.closure() == this);
}
return false;
}

public boolean isUserDataParameterForDestroyNotify() {
return parent().parameters().stream().anyMatch(p ->
p.anyType() instanceof Type type
&& type.get() instanceof Callback
&& p.scope() == Scope.NOTIFIED
&& p.closure() == this
&& p.destroy() != null);
}

public Parameter getRelatedCallbackParameter() {
return parent().parameters().stream().filter(p ->
p.closure() == this).findAny().orElseThrow();
}

public boolean isDestroyNotifyParameter() {
return (anyType() instanceof Type type)
&& "GDestroyNotify".equals(type.cType());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,14 @@

package io.github.jwharm.javagi.test.gio;

import io.github.jwharm.javagi.gobject.SignalConnection;
import org.gnome.gio.Application;
import org.gnome.gio.ApplicationFlags;
import org.gnome.gobject.GObject;
import org.gnome.gobject.GObjects;
import org.junit.jupiter.api.Test;

import java.util.concurrent.atomic.AtomicBoolean;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.*;

/**
* Test connecting a signal, and blocking/unblocking it
Expand All @@ -39,10 +37,8 @@ public class SignalTest {
public void connectSignal() {
var success = new AtomicBoolean(false);
Application app = new Application("test.id1", ApplicationFlags.DEFAULT_FLAGS);
SignalConnection<GObject.NotifyCallback> signal = app.onNotify("application-id", paramSpec -> {
success.set(true);
});
assertTrue(signal.isConnected());
int handlerId = app.onNotify("application-id", _ -> success.set(true));
assertTrue(GObjects.signalHandlerIsConnected(app, handlerId));
app.setApplicationId("test.id2");
assertTrue(success.get());
}
Expand All @@ -51,11 +47,9 @@ public void connectSignal() {
public void disconnectSignal() {
var success = new AtomicBoolean(true);
Application app = new Application("test.id1", ApplicationFlags.DEFAULT_FLAGS);
SignalConnection<GObject.NotifyCallback> signal = app.onNotify("application-id", paramSpec -> {
success.set(false);
});
signal.disconnect();
assertFalse(signal.isConnected());
int handlerId = app.onNotify("application-id", _ -> success.set(false));
GObjects.signalHandlerDisconnect(app, handlerId);
assertFalse(GObjects.signalHandlerIsConnected(app, handlerId));
app.setApplicationId("test.id2");
assertTrue(success.get());
}
Expand All @@ -64,13 +58,11 @@ public void disconnectSignal() {
public void blockUnblockSignal() {
var success = new AtomicBoolean(true);
Application app = new Application("test.id1", ApplicationFlags.DEFAULT_FLAGS);
SignalConnection<GObject.NotifyCallback> signal = app.onNotify("application-id", paramSpec -> {
success.set(false);
});
signal.block();
int handlerId = app.onNotify("application-id", _ -> success.set(false));
GObjects.signalHandlerBlock(app, handlerId);
app.setApplicationId("test.id2");
assertTrue(success.get());
signal.unblock();
GObjects.signalHandlerUnblock(app, handlerId);
app.setApplicationId("test.id3");
assertFalse(success.get());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/* Java-GI - Java language bindings for GObject-Introspection-based libraries
* Copyright (C) 2022-2024 the Java-GI developers
*
* SPDX-License-Identifier: LGPL-2.1-or-later
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 2.1 of the License, or (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, see <http://www.gnu.org/licenses/>.
*/

package io.github.jwharm.javagi.interop;

import java.lang.foreign.*;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.util.HashMap;
import java.util.Map;

/**
* Keeps a list of open Arenas that will be closed in a DestroyNotify callback.
* The DestroyNotify callback will know which Arena to close, based on the
* hashcode that is passed in the user_data parameter.
*/
public class Arenas {

// Contains all open callback arenas that are closed using DestroyNotify
private static final Map<Integer, Arena> ARENAS = new HashMap<>();

/**
* The upcall stub for the DestroyNotify callback method
*/
public static final MemorySegment CLOSE_CB_SYM;

// Allocate the upcall stub for the DestroyNotify callback method
static {
try {
FunctionDescriptor _fdesc = FunctionDescriptor.ofVoid(ValueLayout.ADDRESS);
MethodHandle _handle = MethodHandles.lookup().findStatic(
Arenas.class, "close_cb", _fdesc.toMethodType());
CLOSE_CB_SYM = Linker.nativeLinker().upcallStub(_handle, _fdesc, Arena.global());
} catch (NoSuchMethodException | IllegalAccessException e) {
throw new RuntimeException(e);
}
}

/**
* This is called by native code when it runs the DestroyNotify callback.
* It will close the accompanying Arena.
*
* @param data pointer to the hashcode of the Arena to close
*/
public static void close_cb(MemorySegment data) {
int hashCode = data.reinterpret(ValueLayout.JAVA_INT.byteSize())
.get(ValueLayout.JAVA_INT, 0);
Arena arena = ARENAS.remove(hashCode);
if (arena != null)
arena.close();
}

/**
* This will add the Arena to the global static list of open arenas, and
* return a pointer to the hashcode of the Arena.
*
* @param arena the Arena to cache
* @return a pointer to the hashcode of the Arena
*/
public static MemorySegment cacheArena(Arena arena) {
int hashCode = arena.hashCode();
ARENAS.put(hashCode, arena);
return arena.allocateFrom(ValueLayout.JAVA_INT, hashCode);
}
}

0 comments on commit c8734ce

Please sign in to comment.