From 189ec064fa5e5ecf6d11f6b7543a9a684f123769 Mon Sep 17 00:00:00 2001 From: nathan Date: Sun, 24 Apr 2016 17:32:07 +0200 Subject: [PATCH] Updated GTK threading model to *correctly* execute GTK methods on the GTK thread. Solves problems (unsurprisingly) with race conditions against the native GTK thread. --- .../systemTray/linux/GtkMenuEntry.java | 12 +-- .../systemTray/linux/GtkSystemTray.java | 23 +++-- .../systemTray/linux/GtkTypeSystemTray.java | 16 --- src/dorkbox/systemTray/linux/jna/Gobject.java | 18 +++- src/dorkbox/systemTray/linux/jna/Gtk.java | 9 +- .../systemTray/linux/jna/GtkSupport.java | 99 ++++++++----------- 6 files changed, 81 insertions(+), 96 deletions(-) diff --git a/src/dorkbox/systemTray/linux/GtkMenuEntry.java b/src/dorkbox/systemTray/linux/GtkMenuEntry.java index 7c1fbad..318f870 100644 --- a/src/dorkbox/systemTray/linux/GtkMenuEntry.java +++ b/src/dorkbox/systemTray/linux/GtkMenuEntry.java @@ -79,13 +79,11 @@ class GtkMenuEntry implements MenuEntry, GCallback { int callback(final Pointer instance, final Pointer data) { final SystemTrayMenuAction cb = this.callback; if (cb != null) { - GtkTypeSystemTray.callbackExecutor.execute(new Runnable() { - @Override - public - void run() { - cb.onClick(parent, GtkMenuEntry.this); - } - }); + try { + cb.onClick(parent, GtkMenuEntry.this); + } catch (Throwable throwable) { + throwable.printStackTrace(); + } } return Gtk.TRUE; diff --git a/src/dorkbox/systemTray/linux/GtkSystemTray.java b/src/dorkbox/systemTray/linux/GtkSystemTray.java index 1bb2cae..00d39db 100644 --- a/src/dorkbox/systemTray/linux/GtkSystemTray.java +++ b/src/dorkbox/systemTray/linux/GtkSystemTray.java @@ -21,6 +21,8 @@ import dorkbox.systemTray.linux.jna.Gobject; import dorkbox.systemTray.linux.jna.Gtk; import dorkbox.systemTray.linux.jna.GtkSupport; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -32,11 +34,8 @@ public class GtkSystemTray extends GtkTypeSystemTray { private Pointer trayIcon; - // have to make this a field, to prevent GC on this object - @SuppressWarnings("FieldCanBeLocal") - private Gobject.GEventCallback gtkCallback; - @SuppressWarnings({"FieldCanBeLocal", "unused"}) - private NativeLong button_press_event; + // have to save these in a field to prevent GC on the objects (since they go out-of-scope from java) + private final List gtkCallbacks = new ArrayList(); // This is required if we have JavaFX or SWT shutdown hooks (to prevent us from shutting down twice...) private AtomicBoolean shuttingDown = new AtomicBoolean(); @@ -53,12 +52,12 @@ class GtkSystemTray extends GtkTypeSystemTray { public void run() { final Pointer trayIcon_ = gtk.gtk_status_icon_new(); - gtk.gtk_status_icon_set_title(trayIcon_, "SystemTray@Dorkbox"); + gtk.gtk_status_icon_set_title(trayIcon_, GnomeShellExtension.UID); gtk.gtk_status_icon_set_name(trayIcon_, "SystemTray"); trayIcon = trayIcon_; - gtkCallback = new Gobject.GEventCallback() { + final Gobject.GEventCallback gtkCallback = new Gobject.GEventCallback() { @Override public void callback(Pointer notUsed, final Gtk.GdkEventButton event) { @@ -68,7 +67,11 @@ class GtkSystemTray extends GtkTypeSystemTray { } } }; - button_press_event = gobject.g_signal_connect_object(trayIcon, "button_press_event", gtkCallback, null, 0); + final NativeLong button_press_event = gobject.g_signal_connect_object(trayIcon, "button_press_event", gtkCallback, null, 0); + + // have to do this to prevent GC on these objects + gtkCallbacks.add(gtkCallback); + gtkCallbacks.add(button_press_event); } }); } @@ -86,9 +89,9 @@ class GtkSystemTray extends GtkTypeSystemTray { gtk.gtk_status_icon_set_visible(trayIcon, false); gobject.g_object_unref(trayIcon); - // GC it + // mark for GC trayIcon = null; - gtkCallback = null; + gtkCallbacks.clear(); } }); diff --git a/src/dorkbox/systemTray/linux/GtkTypeSystemTray.java b/src/dorkbox/systemTray/linux/GtkTypeSystemTray.java index 589c14b..d1883da 100644 --- a/src/dorkbox/systemTray/linux/GtkTypeSystemTray.java +++ b/src/dorkbox/systemTray/linux/GtkTypeSystemTray.java @@ -23,13 +23,9 @@ import dorkbox.systemTray.SystemTrayMenuAction; import dorkbox.systemTray.linux.jna.Gobject; import dorkbox.systemTray.linux.jna.Gtk; import dorkbox.systemTray.linux.jna.GtkSupport; -import dorkbox.util.NamedThreadFactory; import java.io.InputStream; import java.net.URL; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; /** * Derived from @@ -40,8 +36,6 @@ class GtkTypeSystemTray extends SystemTray { protected static final Gobject gobject = Gobject.INSTANCE; protected static final Gtk gtk = Gtk.INSTANCE; - final static ExecutorService callbackExecutor = Executors.newSingleThreadExecutor(new NamedThreadFactory("SysTrayExecutor", false)); - private volatile Pointer menu; private volatile Pointer connectionStatusItem; @@ -62,16 +56,6 @@ class GtkTypeSystemTray extends SystemTray { void run() { obliterateMenu(); - boolean terminated = false; - try { - terminated = callbackExecutor.awaitTermination(1, TimeUnit.SECONDS); - } catch (InterruptedException ignored) { - } - - if (!terminated) { - callbackExecutor.shutdownNow(); - } - GtkSupport.shutdownGui(); } }); diff --git a/src/dorkbox/systemTray/linux/jna/Gobject.java b/src/dorkbox/systemTray/linux/jna/Gobject.java index 40cf0fe..25671f1 100644 --- a/src/dorkbox/systemTray/linux/jna/Gobject.java +++ b/src/dorkbox/systemTray/linux/jna/Gobject.java @@ -15,9 +15,13 @@ */ package dorkbox.systemTray.linux.jna; -import com.sun.jna.*; +import com.sun.jna.Callback; +import com.sun.jna.Library; +import com.sun.jna.Native; +import com.sun.jna.NativeLong; +import com.sun.jna.Pointer; +import com.sun.jna.Structure; import dorkbox.util.Keep; -import dorkbox.systemTray.linux.jna.Gtk.GdkEventButton; import java.util.Arrays; import java.util.List; @@ -126,6 +130,14 @@ interface Gobject extends Library { } + @Keep + interface FuncCallback extends Callback { + /** + * @return Gtk.FALSE if it will be automatically removed from the stack once it's handled + */ + int callback(Pointer data); + } + @Keep interface GCallback extends Callback { /** @@ -137,7 +149,7 @@ interface Gobject extends Library { @Keep interface GEventCallback extends Callback { - void callback(Pointer instance, GdkEventButton event); + void callback(Pointer instance, Gtk.GdkEventButton event); } diff --git a/src/dorkbox/systemTray/linux/jna/Gtk.java b/src/dorkbox/systemTray/linux/jna/Gtk.java index 1d25820..57a03d5 100644 --- a/src/dorkbox/systemTray/linux/jna/Gtk.java +++ b/src/dorkbox/systemTray/linux/jna/Gtk.java @@ -66,8 +66,15 @@ interface Gtk extends Library { */ void gtk_main(); + /** + * using g_idle_add() instead would require thread protection in the callback + * @param callback + * @param data + * @return TRUE to run this callback again, FALSE to remove from the list of event sources (and not call it again) + */ + int gdk_threads_add_idle (Gobject.FuncCallback callback, Pointer data); - /** sks for the current nesting level of the main loop. Useful to determine (at startup) if GTK is already runnign */ + /** aks for the current nesting level of the main loop. Useful to determine (at startup) if GTK is already running */ int gtk_main_level(); /** diff --git a/src/dorkbox/systemTray/linux/jna/GtkSupport.java b/src/dorkbox/systemTray/linux/jna/GtkSupport.java index abd8c5b..48eb3f0 100644 --- a/src/dorkbox/systemTray/linux/jna/GtkSupport.java +++ b/src/dorkbox/systemTray/linux/jna/GtkSupport.java @@ -17,23 +17,31 @@ package dorkbox.systemTray.linux.jna; import com.sun.jna.Function; import com.sun.jna.Native; +import com.sun.jna.Pointer; import dorkbox.systemTray.SystemTray; -import java.util.concurrent.ArrayBlockingQueue; +import java.util.LinkedList; import java.util.concurrent.CountDownLatch; public class GtkSupport { + // For funsies, SyncThing did a LOT of work on compatibility (unfortunate for us) in python. + // https://github.com/syncthing/syncthing-gtk/blob/b7a3bc00e3bb6d62365ae62b5395370f3dcc7f55/syncthing_gtk/statusicon.py + private static volatile boolean started = false; - private static final ArrayBlockingQueue dispatchEvents = new ArrayBlockingQueue(256); - private static volatile Thread gtkDispatchThread; + + // have to save these in a field to prevent GC on the objects (since they go out-of-scope from java) + private static final LinkedList gtkCallbacks = new LinkedList(); /** * must call get() before accessing this! Only "Gtk" interface should access this! */ static volatile Function gtk_status_icon_position_menu = null; + public static volatile boolean isGtk2 = false; + private static volatile boolean alreadyRunningGTK = false; + private static Thread gtkUpdateThread = null; /** * Helper for GTK, because we could have v3 or v2. @@ -130,7 +138,7 @@ class GtkSupport { } catch (Throwable ignored) { } - throw new RuntimeException("We apologize for this, but we are unable to determine the GTK library is in use, if " + + throw new RuntimeException("We apologize for this, but we are unable to determine the GTK library is in use, " + "or even if it is in use... Please create an issue for this and include your OS type and configuration."); } @@ -140,43 +148,14 @@ class GtkSupport { if (!started) { started = true; - // GTK specifies that we ONLY run from a single thread. This guarantees that. - gtkDispatchThread = new Thread() { - @Override - public - void run() { - final Gtk gtk = Gtk.INSTANCE; - while (started) { - try { - final Runnable take = dispatchEvents.take(); - - gtk.gdk_threads_enter(); - - try { - take.run(); - } catch (Throwable e) { - e.printStackTrace(); - } - - gtk.gdk_threads_leave(); - - } catch (InterruptedException e) { - e.printStackTrace(); - } - } - } - }; - gtkDispatchThread.setName("GTK Event Loop"); - gtkDispatchThread.start(); - - // startup the GTK GUI event loop. There can be multiple/nested loops. // If JavaFX/SWT is used, this is UNNECESSARY if (!alreadyRunningGTK) { // only necessary if we are the only GTK instance running... final CountDownLatch blockUntilStarted = new CountDownLatch(1); - Thread gtkUpdateThread = new Thread() { + + gtkUpdateThread = new Thread() { @Override public void run() { @@ -191,6 +170,7 @@ class GtkSupport { if (!SystemTray.COMPATIBILITY_MODE) { gtk.gtk_init_check(0, null); } + // notify our main thread to continue blockUntilStarted.countDown(); @@ -202,7 +182,7 @@ class GtkSupport { gtk.gdk_threads_leave(); } }; - gtkUpdateThread.setName("GTK Event Loop (Native)"); + gtkUpdateThread.setName("GTK Native Event Loop"); gtkUpdateThread.start(); try { @@ -216,40 +196,41 @@ class GtkSupport { } /** - * Best practices for GTK, is to call EVERYTHING for it on a SINGLE THREAD. This accomplishes that. + * Best practices for GTK, is to call EVERYTHING for it on the GTK THREAD. This accomplishes that. */ public static - void dispatch(Runnable runnable) { - try { - dispatchEvents.put(runnable); - } catch (InterruptedException e) { - e.printStackTrace(); + void dispatch(final Runnable runnable) { + if (gtkUpdateThread == Thread.currentThread()) { + // if we are ALREADY inside the native event + runnable.run(); + } else { + final Gobject.FuncCallback callback = new Gobject.FuncCallback() { + @Override + public + int callback(final Pointer data) { + synchronized (gtkCallbacks) { + gtkCallbacks.removeFirst(); // now that we've 'handled' it, we can remove it from our callback list + } + runnable.run(); + + return Gtk.FALSE; // don't want to call this again + } + }; + + synchronized (gtkCallbacks) { + gtkCallbacks.offer(callback); // prevent GC from collecting this object before it can be called + } + Gtk.INSTANCE.gdk_threads_add_idle(callback, null); } } public static void shutdownGui() { - // If JavaFX/SWT is used, this is UNNECESSARY (an will break SWT/JavaFX shutdown) + // If JavaFX/SWT is used, this is UNNECESSARY (and will break SWT/JavaFX shutdown) if (!(alreadyRunningGTK || SystemTray.COMPATIBILITY_MODE)) { Gtk.INSTANCE.gtk_main_quit(); } started = false; - - // put it in a NEW dispatch event (so that we cleanup AFTER this one is finished) - dispatch(new Runnable() { - @Override - public - void run() { - new Thread(new Runnable() { - @Override - public - void run() { - // this should happen in a new thread - gtkDispatchThread.interrupt(); - } - }).run(); - } - }); } }