From 53acb194bc22ffb05d3f5590dd16e198c6bc4a53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 15 Sep 2026 11:55:40 +0200 Subject: [PATCH 1/7] Keep Go calls off the Go callback thread and the main thread The Go engine delivers state events on its own threads. The home screen answered onPeersListChanged by calling getNetworks() right there, which crosses back into Go from the callback thread, and SessionMonitor polled status() and sessionExpiresAt() on the main thread on every state change. Under a peer storm both queue up on the engine's status lock; an ANR report showed the main thread stuck behind 283 such callback threads. Add CoalescingWorker, a single background thread that collapses bursts of refresh requests into one run, and route the engine reads through it: the exit node row and the session deadline seed on the home screen, the networks list, the session monitor, and the exit node picker. Callbacks only signal; results are posted back to the main thread. Debug builds now log a stack trace when networks(), peersInfo() or sessionExpiresAt() is called on the main thread. --- .../client/ui/home/ExitNodePickerSheet.java | 26 +++++- .../netbird/client/ui/home/HomeFragment.java | 44 +++++++++-- .../ui/home/NetworksFragmentViewModel.java | 10 ++- .../netbird/client/tool/CoalescingWorker.java | 58 ++++++++++++++ .../io/netbird/client/tool/EngineRunner.java | 14 ++++ .../netbird/client/tool/SessionMonitor.java | 49 +++++++----- .../client/tool/CoalescingWorkerUnitTest.java | 79 +++++++++++++++++++ 7 files changed, 253 insertions(+), 27 deletions(-) create mode 100644 tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java create mode 100644 tool/src/test/java/io/netbird/client/tool/CoalescingWorkerUnitTest.java diff --git a/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java b/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java index c107dad0..b4c34b19 100644 --- a/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java +++ b/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java @@ -21,6 +21,7 @@ import io.netbird.client.R; import io.netbird.client.ServiceAccessor; import io.netbird.client.databinding.SheetExitNodePickerBinding; +import io.netbird.client.tool.CoalescingWorker; import io.netbird.gomobile.android.NetworkArray; /** @@ -34,9 +35,11 @@ public class ExitNodePickerSheet extends BottomSheetDialogFragment { private static final String TAG = "ExitNodePickerSheet"; private SheetExitNodePickerBinding binding; - private ServiceAccessor serviceAccessor; + private volatile ServiceAccessor serviceAccessor; private final List entries = new ArrayList<>(); private ExitNodePickerAdapter adapter; + // getNetworks() is a JNI call into Go; keep it off the main thread. + private final CoalescingWorker exitNodesLoader = new CoalescingWorker("nb-exit-node-picker", this::loadExitNodes); @Override public void onAttach(@NonNull Context context) { @@ -64,7 +67,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat list.setLayoutManager(new LinearLayoutManager(requireContext())); list.setAdapter(adapter); - loadExitNodes(); + exitNodesLoader.request(); } @Override @@ -79,8 +82,15 @@ public void onDetach() { serviceAccessor = null; } + @Override + public void onDestroy() { + exitNodesLoader.shutdown(); + super.onDestroy(); + } + private void loadExitNodes() { - NetworkArray networks = serviceAccessor != null ? serviceAccessor.getNetworks() : null; + ServiceAccessor accessor = serviceAccessor; + NetworkArray networks = accessor != null ? accessor.getNetworks() : null; List nodes = new ArrayList<>(); boolean anySelected = false; @@ -96,6 +106,16 @@ private void loadExitNodes() { } } + View root = binding != null ? binding.getRoot() : null; + if (root != null) { + root.post(() -> showExitNodes(nodes, anySelected)); + } + } + + private void showExitNodes(List nodes, boolean anySelected) { + if (binding == null || adapter == null) { + return; + } entries.clear(); entries.add(new ExitNodePickerAdapter.Entry( null, getString(R.string.exit_node_picker_none), !anySelected)); diff --git a/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java b/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java index c438166d..f24be4d2 100644 --- a/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java +++ b/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java @@ -30,6 +30,7 @@ import io.netbird.client.StateListener; import io.netbird.client.StateListenerRegistry; import io.netbird.client.databinding.FragmentHomeBinding; +import io.netbird.client.tool.CoalescingWorker; import io.netbird.client.tool.Profile; import io.netbird.client.tool.ProfileManagerWrapper; import io.netbird.client.tool.RouteChangeListener; @@ -42,7 +43,7 @@ public class HomeFragment extends Fragment implements StateListener, RouteChange private FragmentHomeBinding binding; // Set only while the addresses are floating; see toggleInfoRows. private PopupWindow addressPopup; - private ServiceAccessor serviceAccessor; + private volatile ServiceAccessor serviceAccessor; private StateListenerRegistry stateListenerRegistry; private TextView textHostname; @@ -51,7 +52,10 @@ public class HomeFragment extends Fragment implements StateListener, RouteChange private TextView textConnStatus; private SwitchMaterial buttonConnect; - private boolean isConnected; + private volatile boolean isConnected; + // getNetworks() and sessionExpiresAt() are JNI calls into Go; they must not run + // on the Go callback thread that delivers the state events, nor on the main thread. + private final CoalescingWorker engineQueries = new CoalescingWorker("nb-home-engine", this::refreshExitNodeRow); /** * Pulses the toggle while it is disabled, matching the desktop client. Held so it can @@ -98,6 +102,9 @@ private enum EngineState { CONNECTING, CONNECTED, DISCONNECTING, DISCONNECTED, N private static final long DAY_MS = 24 * HOUR_MS; private long sessionDeadlineUnixSeconds; + // Bumped on every deadline push so a slower background seed cannot overwrite a + // newer value; an extend finishing during the SSO round-trip lands right at onResume. + private volatile int sessionDeadlineVersion; // The management server rejected the peer, so reconnecting needs a login. // Outlives the engine (and the app process), so it is reported on bind as // well as when it happens — and it overrides the disconnected label, which @@ -239,9 +246,7 @@ public void onResume() { updateProfileChip(); // Deadline changes arrive via onSessionDeadlineChanged while resumed; // this seeds the value after a (re)bind or a return to the screen. - if (serviceAccessor != null) { - sessionDeadlineUnixSeconds = serviceAccessor.sessionExpiresAt(); - } + seedSessionDeadline(); updateSessionRow(); if (binding != null) { binding.getRoot().removeCallbacks(sessionTicker); @@ -683,6 +688,12 @@ public void onDetach() { serviceAccessor = null; } + @Override + public void onDestroy() { + engineQueries.shutdown(); + super.onDestroy(); + } + @Override public void onEngineStarted() { @@ -705,6 +716,10 @@ public void onRouteChanged(String routes) { * The subtitle names the active exit node, if any. */ private void updateExitNodeRow() { + engineQueries.request(); + } + + private void refreshExitNodeRow() { ServiceAccessor accessor = serviceAccessor; if (accessor == null || binding == null) { return; @@ -838,6 +853,7 @@ public void onPeersListChanged(long numberOfPeers) { @Override public void onSessionDeadlineChanged(long expiresAtUnixSeconds) { + sessionDeadlineVersion++; sessionDeadlineUnixSeconds = expiresAtUnixSeconds; runOnUi(this::updateSessionRow); } @@ -853,6 +869,24 @@ public void onLoginRequired() { } } + private void seedSessionDeadline() { + ServiceAccessor accessor = serviceAccessor; + if (accessor == null) { + return; + } + int version = sessionDeadlineVersion; + engineQueries.submit(() -> { + long deadline = accessor.sessionExpiresAt(); + runOnUi(() -> { + if (sessionDeadlineVersion != version) { + return; + } + sessionDeadlineUnixSeconds = deadline; + updateSessionRow(); + }); + }); + } + private void updateSessionRow() { if (binding == null) { return; diff --git a/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java b/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java index 5da6bac4..f64b6d2b 100644 --- a/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java +++ b/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java @@ -10,6 +10,7 @@ import io.netbird.client.ServiceAccessor; import io.netbird.client.StateListener; +import io.netbird.client.tool.CoalescingWorker; import io.netbird.client.tool.RouteChangeListener; import io.netbird.gomobile.android.NetworkArray; import io.netbird.gomobile.android.NetworkDomains; @@ -24,6 +25,9 @@ public class NetworksFragmentViewModel extends ViewModel implements RouteChangeL // captured at construction would keep reading through a dead Activity's unbound // service connection forever. private volatile ServiceAccessor serviceAccessor; + // getNetworks()/getPeersList() are JNI calls into Go; keep them off the Go + // callback thread that delivers route and peer events. + private final CoalescingWorker resourcesRefresher = new CoalescingWorker("nb-networks", this::loadResources); private final MutableLiveData uiState = new MutableLiveData<>(new NetworksFragmentUiState(new ArrayList<>(), new ArrayList<>())); @@ -48,6 +52,7 @@ public void setServiceAccessor(@Nullable ServiceAccessor accessor) { @Override protected void onCleared() { super.onCleared(); + resourcesRefresher.shutdown(); setServiceAccessor(null); } @@ -138,6 +143,10 @@ private List getRoutingPeers(PeerInfoArray peersFromEngine) { } private void postResources() { + resourcesRefresher.request(); + } + + private void loadResources() { ServiceAccessor accessor = serviceAccessor; if (accessor == null) { return; @@ -152,7 +161,6 @@ private void postResources() { return; } - // This value will be set from a background thread. uiState.postValue(new NetworksFragmentUiState(getNetworks(networks), getRoutingPeers(peersFromEngine))); } diff --git a/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java b/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java new file mode 100644 index 00000000..f536d20e --- /dev/null +++ b/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java @@ -0,0 +1,58 @@ +package io.netbird.client.tool; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Runs one fixed task on a single background thread. Requests made while a run + * is already queued collapse into that run; a request made during a run queues + * exactly one more, so the task always ends on the latest state. Meant for + * refreshes triggered by Go callbacks: the callback thread only signals, the + * JNI calls back into Go happen here. + */ +public final class CoalescingWorker { + private final ExecutorService executor; + private final Runnable task; + private final AtomicBoolean queued = new AtomicBoolean(false); + + public CoalescingWorker(String threadName, Runnable task) { + this.executor = Executors.newSingleThreadExecutor(r -> { + Thread t = new Thread(r, threadName); + t.setDaemon(true); + return t; + }); + this.task = task; + } + + /** Schedules a run unless one is already waiting. Safe from any thread. */ + public void request() { + if (queued.getAndSet(true)) { + return; + } + try { + executor.execute(this::run); + } catch (RejectedExecutionException ignored) { + queued.set(false); + } + } + + /** Runs a one-off action on the worker thread, after any queued run. */ + public void submit(Runnable action) { + try { + executor.execute(action); + } catch (RejectedExecutionException ignored) { + // shut down; the owner is going away and no longer wants the result + } + } + + public void shutdown() { + executor.shutdown(); + } + + private void run() { + queued.set(false); + task.run(); + } +} diff --git a/tool/src/main/java/io/netbird/client/tool/EngineRunner.java b/tool/src/main/java/io/netbird/client/tool/EngineRunner.java index ec68f97e..061929ac 100644 --- a/tool/src/main/java/io/netbird/client/tool/EngineRunner.java +++ b/tool/src/main/java/io/netbird/client/tool/EngineRunner.java @@ -2,6 +2,7 @@ import android.content.Context; import android.os.Build; +import android.os.Looper; import android.util.Log; import org.jetbrains.annotations.NotNull; @@ -91,6 +92,7 @@ public String status() { /** Session deadline as unix seconds, or 0 when none is known. */ public long sessionExpiresAt() { + warnIfOnMainThread("sessionExpiresAt"); return goClient.sessionExpiresAtUnix(); } @@ -351,11 +353,13 @@ public synchronized void stop() { } public PeerInfoArray peersInfo() { + warnIfOnMainThread("peersInfo"); return goClient.peersList(); } @Nullable public NetworkArray networks() { + warnIfOnMainThread("networks"); NetworkArray networks = goClient.networks(); if (networks == null) { Log.e(LOGTAG, "Failed to retrieve networks"); @@ -379,6 +383,16 @@ private synchronized void notifyServiceStateListeners(boolean engineIsRunning) { } } + // These calls take the engine's status lock, which a network map update or a + // peer storm can hold for a long time. Debug builds flag callers that would + // let that block the UI thread. + private void warnIfOnMainThread(String call) { + if (!isDebuggable || Looper.myLooper() != Looper.getMainLooper()) { + return; + } + Log.w(LOGTAG, "Go call " + call + " on the main thread", new Throwable()); + } + private void updateLogLevel(boolean isTraceLogEnabled, boolean isDebuggable) { if (isDebuggable || isTraceLogEnabled) { goClient.setTraceLogLevel(); diff --git a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java index ccc6ba55..8d7513b5 100644 --- a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java +++ b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java @@ -15,8 +15,10 @@ * publishes the warnings; this class only forwards them and edge-detects the * NeedsLogin status label, which the run loop sets outside the event stream. * - *

All state lives on the main thread; {@link #onStateChanged()} and - * {@link #onSessionExpiring} may be called from any thread. + *

The Go calls run on a single worker thread so a busy engine never stalls + * the main thread; listener callbacks are posted to the main thread. + * {@link #onStateChanged()} and {@link #onSessionExpiring} may be called from + * any thread. */ public class SessionMonitor { @@ -29,6 +31,7 @@ public class SessionMonitor { private final Supplier status; private final LongSupplier deadlineUnixSeconds; private final Set listeners = ConcurrentHashMap.newKeySet(); + private final CoalescingWorker refresher = new CoalescingWorker("nb-session-monitor", this::refresh); private long lastDeadline; private boolean wasNeedsLogin; @@ -40,7 +43,7 @@ public SessionMonitor(Supplier status, LongSupplier deadlineUnixSeconds) /** Wake-up from the Go state-change signal. Safe to call from any thread. */ public void onStateChanged() { - handler.post(this::refresh); + refresher.request(); } /** Expiry warning from the engine's session watcher. Any thread. */ @@ -60,12 +63,16 @@ public void addListener(SessionEventListener listener) { // moves the deadline), and a session can expire with no UI running at // all — the expiry is an edge the listener would never see otherwise. // Same pattern as the Go notifier's setListener. - handler.post(() -> { + refresher.submit(() -> { refresh(); - notifySafely(() -> listener.onSessionDeadlineChanged(lastDeadline)); - if (isLoginRequired()) { - notifySafely(listener::onSessionExpired); - } + long deadline = lastDeadline; + boolean needsLogin = wasNeedsLogin; + handler.post(() -> { + notifySafely(() -> listener.onSessionDeadlineChanged(deadline)); + if (needsLogin) { + notifySafely(listener::onSessionExpired); + } + }); }); } @@ -80,20 +87,26 @@ public void removeListener(SessionEventListener listener) { private void refresh() { long deadline = deadlineUnixSeconds.getAsLong(); - if (deadline != lastDeadline) { - lastDeadline = deadline; - for (SessionEventListener l : listeners) { - notifySafely(() -> l.onSessionDeadlineChanged(deadline)); - } - } + boolean deadlineChanged = deadline != lastDeadline; + lastDeadline = deadline; boolean needsLogin = STATUS_NEEDS_LOGIN.equals(status.get()); - if (needsLogin && !wasNeedsLogin) { + boolean expired = needsLogin && !wasNeedsLogin; + wasNeedsLogin = needsLogin; + + if (!deadlineChanged && !expired) { + return; + } + handler.post(() -> { for (SessionEventListener l : listeners) { - notifySafely(l::onSessionExpired); + if (deadlineChanged) { + notifySafely(() -> l.onSessionDeadlineChanged(deadline)); + } + if (expired) { + notifySafely(l::onSessionExpired); + } } - } - wasNeedsLogin = needsLogin; + }); } private void notifySafely(Runnable r) { diff --git a/tool/src/test/java/io/netbird/client/tool/CoalescingWorkerUnitTest.java b/tool/src/test/java/io/netbird/client/tool/CoalescingWorkerUnitTest.java new file mode 100644 index 00000000..4d6f4d81 --- /dev/null +++ b/tool/src/test/java/io/netbird/client/tool/CoalescingWorkerUnitTest.java @@ -0,0 +1,79 @@ +package io.netbird.client.tool; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +public class CoalescingWorkerUnitTest { + + @Test + public void requestsDuringRunCollapseIntoOneMoreRun() throws Exception { + CountDownLatch firstRunStarted = new CountDownLatch(1); + CountDownLatch releaseFirstRun = new CountDownLatch(1); + CountDownLatch twoRunsDone = new CountDownLatch(2); + AtomicInteger runs = new AtomicInteger(); + + CoalescingWorker worker = new CoalescingWorker("test-worker", () -> { + if (runs.incrementAndGet() == 1) { + firstRunStarted.countDown(); + await(releaseFirstRun); + } + twoRunsDone.countDown(); + }); + + worker.request(); + assertTrue(firstRunStarted.await(5, TimeUnit.SECONDS)); + for (int i = 0; i < 100; i++) { + worker.request(); + } + releaseFirstRun.countDown(); + + assertTrue(twoRunsDone.await(5, TimeUnit.SECONDS)); + Thread.sleep(100); + assertEquals(2, runs.get()); + worker.shutdown(); + } + + @Test + public void taskRunsOnTheWorkerThread() throws Exception { + CountDownLatch done = new CountDownLatch(1); + AtomicReference threadName = new AtomicReference<>(); + + CoalescingWorker worker = new CoalescingWorker("nb-test", () -> { + threadName.set(Thread.currentThread().getName()); + done.countDown(); + }); + + worker.request(); + assertTrue(done.await(5, TimeUnit.SECONDS)); + assertEquals("nb-test", threadName.get()); + worker.shutdown(); + } + + @Test + public void requestsAfterShutdownAreDropped() throws Exception { + AtomicInteger runs = new AtomicInteger(); + CoalescingWorker worker = new CoalescingWorker("test-worker", runs::incrementAndGet); + + worker.shutdown(); + worker.request(); + worker.submit(runs::incrementAndGet); + Thread.sleep(100); + + assertEquals(0, runs.get()); + } + + private static void await(CountDownLatch latch) { + try { + latch.await(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } +} From 1ad266729ce8c7a46b160246779b9b3f65a21a5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 15 Sep 2026 12:01:22 +0200 Subject: [PATCH 2/7] Capture the exit node selection in a final local for the UI lambda --- .../java/io/netbird/client/ui/home/ExitNodePickerSheet.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java b/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java index b4c34b19..87b07eb8 100644 --- a/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java +++ b/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java @@ -106,9 +106,10 @@ private void loadExitNodes() { } } + final boolean selected = anySelected; View root = binding != null ? binding.getRoot() : null; if (root != null) { - root.post(() -> showExitNodes(nodes, anySelected)); + root.post(() -> showExitNodes(nodes, selected)); } } From a929024b29e730f022043583f65aef6e2105e4bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 15 Sep 2026 13:05:36 +0200 Subject: [PATCH 3/7] Tighten the background refresh paths from review - NetworksFragmentViewModel: a load that started before a disconnect could publish its snapshot after clearResources() emptied the list. Guard the publish with a generation counter that clearResources() bumps. - SessionMonitor: the worker thread was never stopped. Add shutdown() and call it from VPNService.onDestroy(). - SessionMonitor: the replay after addListener() could deliver a deadline or expiry callback twice when the refresh in the same action already sent it, and could notify a listener removed before the main thread ran. Replay only what the refresh did not dispatch, and skip listeners no longer registered. --- .../ui/home/NetworksFragmentViewModel.java | 22 ++++++++++-- .../netbird/client/tool/SessionMonitor.java | 34 ++++++++++++++++--- .../io/netbird/client/tool/VPNService.java | 1 + 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java b/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java index f64b6d2b..32e5ad4c 100644 --- a/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java +++ b/app/src/main/java/io/netbird/client/ui/home/NetworksFragmentViewModel.java @@ -28,6 +28,10 @@ public class NetworksFragmentViewModel extends ViewModel implements RouteChangeL // getNetworks()/getPeersList() are JNI calls into Go; keep them off the Go // callback thread that delivers route and peer events. private final CoalescingWorker resourcesRefresher = new CoalescingWorker("nb-networks", this::loadResources); + // Bumped by clearResources so a load that started before a disconnect cannot + // repopulate the list after it was cleared. + private final Object publishLock = new Object(); + private int resourcesGeneration; private final MutableLiveData uiState = new MutableLiveData<>(new NetworksFragmentUiState(new ArrayList<>(), new ArrayList<>())); @@ -147,6 +151,10 @@ private void postResources() { } private void loadResources() { + int generation; + synchronized (publishLock) { + generation = resourcesGeneration; + } ServiceAccessor accessor = serviceAccessor; if (accessor == null) { return; @@ -161,7 +169,14 @@ private void loadResources() { return; } - uiState.postValue(new NetworksFragmentUiState(getNetworks(networks), getRoutingPeers(peersFromEngine))); + NetworksFragmentUiState state = + new NetworksFragmentUiState(getNetworks(networks), getRoutingPeers(peersFromEngine)); + synchronized (publishLock) { + if (generation != resourcesGeneration) { + return; + } + uiState.postValue(state); + } } @Override @@ -170,7 +185,10 @@ public void onRouteChanged(String routes) { } private void clearResources() { - uiState.postValue(new NetworksFragmentUiState(new ArrayList<>(), new ArrayList<>())); + synchronized (publishLock) { + resourcesGeneration++; + uiState.postValue(new NetworksFragmentUiState(new ArrayList<>(), new ArrayList<>())); + } } public void selectRoute(String route) throws Exception { diff --git a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java index 8d7513b5..7da96330 100644 --- a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java +++ b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java @@ -36,11 +36,26 @@ public class SessionMonitor { private long lastDeadline; private boolean wasNeedsLogin; + private static final class Refresh { + final boolean deadlineChanged; + final boolean expired; + + Refresh(boolean deadlineChanged, boolean expired) { + this.deadlineChanged = deadlineChanged; + this.expired = expired; + } + } + public SessionMonitor(Supplier status, LongSupplier deadlineUnixSeconds) { this.status = status; this.deadlineUnixSeconds = deadlineUnixSeconds; } + /** Stops the worker thread. Call when the owning service is destroyed. */ + public void shutdown() { + refresher.shutdown(); + } + /** Wake-up from the Go state-change signal. Safe to call from any thread. */ public void onStateChanged() { refresher.request(); @@ -64,12 +79,19 @@ public void addListener(SessionEventListener listener) { // all — the expiry is an edge the listener would never see otherwise. // Same pattern as the Go notifier's setListener. refresher.submit(() -> { - refresh(); + // The listener is already registered, so whatever this refresh + // dispatches reaches it; replay only what it did not. + Refresh dispatched = refresh(); long deadline = lastDeadline; boolean needsLogin = wasNeedsLogin; handler.post(() -> { - notifySafely(() -> listener.onSessionDeadlineChanged(deadline)); - if (needsLogin) { + if (!listeners.contains(listener)) { + return; + } + if (!dispatched.deadlineChanged) { + notifySafely(() -> listener.onSessionDeadlineChanged(deadline)); + } + if (needsLogin && !dispatched.expired) { notifySafely(listener::onSessionExpired); } }); @@ -85,7 +107,7 @@ public void removeListener(SessionEventListener listener) { listeners.remove(listener); } - private void refresh() { + private Refresh refresh() { long deadline = deadlineUnixSeconds.getAsLong(); boolean deadlineChanged = deadline != lastDeadline; lastDeadline = deadline; @@ -94,8 +116,9 @@ private void refresh() { boolean expired = needsLogin && !wasNeedsLogin; wasNeedsLogin = needsLogin; + Refresh result = new Refresh(deadlineChanged, expired); if (!deadlineChanged && !expired) { - return; + return result; } handler.post(() -> { for (SessionEventListener l : listeners) { @@ -107,6 +130,7 @@ private void refresh() { } } }); + return result; } private void notifySafely(Runnable r) { diff --git a/tool/src/main/java/io/netbird/client/tool/VPNService.java b/tool/src/main/java/io/netbird/client/tool/VPNService.java index eed792d9..6369f553 100644 --- a/tool/src/main/java/io/netbird/client/tool/VPNService.java +++ b/tool/src/main/java/io/netbird/client/tool/VPNService.java @@ -205,6 +205,7 @@ public void onDestroy() { networkChangeDetector.unsubscribe(); networkChangeDetector.unregisterNetworkCallback(); + sessionMonitor.shutdown(); engineRunner.stop(); stopForeground(true); From 1f811aab35ca6c323e994cae0d486beb84a4c717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 15 Sep 2026 13:31:03 +0200 Subject: [PATCH 4/7] Stop session callbacks after shutdown and register listeners on the worker - SessionMonitor.shutdown() now sets a closing flag and drops the listeners; refresh() and every posted callback exit once closed, so a destroyed service no longer receives session events. CoalescingWorker drops queued runs on shutdown instead of draining them. - addListener() registers the listener on the worker after the refresh queued ahead of it, and refresh() dispatches to a snapshot of the listeners taken at that moment. A change dispatched by an earlier refresh can no longer reach the new listener before its replay, so the replay delivers the current state exactly once. removeListener() goes through the worker too, keeping add and remove ordered. --- .../netbird/client/tool/CoalescingWorker.java | 3 +- .../netbird/client/tool/SessionMonitor.java | 56 ++++++++++--------- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java b/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java index f536d20e..0e57f6c0 100644 --- a/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java +++ b/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java @@ -47,8 +47,9 @@ public void submit(Runnable action) { } } + /** Stops the worker. Queued runs are dropped; a run in progress finishes. */ public void shutdown() { - executor.shutdown(); + executor.shutdownNow(); } private void run() { diff --git a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java index 7da96330..4a8406e6 100644 --- a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java +++ b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java @@ -4,6 +4,8 @@ import android.os.Looper; import android.util.Log; +import java.util.ArrayList; +import java.util.List; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.LongSupplier; @@ -32,27 +34,20 @@ public class SessionMonitor { private final LongSupplier deadlineUnixSeconds; private final Set listeners = ConcurrentHashMap.newKeySet(); private final CoalescingWorker refresher = new CoalescingWorker("nb-session-monitor", this::refresh); + private volatile boolean closed; private long lastDeadline; private boolean wasNeedsLogin; - private static final class Refresh { - final boolean deadlineChanged; - final boolean expired; - - Refresh(boolean deadlineChanged, boolean expired) { - this.deadlineChanged = deadlineChanged; - this.expired = expired; - } - } - public SessionMonitor(Supplier status, LongSupplier deadlineUnixSeconds) { this.status = status; this.deadlineUnixSeconds = deadlineUnixSeconds; } - /** Stops the worker thread. Call when the owning service is destroyed. */ + /** Stops the worker and drops every listener. Call when the owning service is destroyed. */ public void shutdown() { + closed = true; + listeners.clear(); refresher.shutdown(); } @@ -64,6 +59,9 @@ public void onStateChanged() { /** Expiry warning from the engine's session watcher. Any thread. */ public void onSessionExpiring(long expiresAtUnixSeconds, long leadMinutes, boolean finalWarning) { handler.post(() -> { + if (closed) { + return; + } for (SessionEventListener l : listeners) { notifySafely(() -> l.onSessionExpiring(expiresAtUnixSeconds, leadMinutes, finalWarning)); } @@ -71,27 +69,28 @@ public void onSessionExpiring(long expiresAtUnixSeconds, long leadMinutes, boole } public void addListener(SessionEventListener listener) { - listeners.add(listener); // Replay the current state so a late-binding UI doesn't miss what // happened while it was detached. Both cases are real: the activity // unbinds for the SSO browser round-trip (which is when an extend // moves the deadline), and a session can expire with no UI running at // all — the expiry is an edge the listener would never see otherwise. // Same pattern as the Go notifier's setListener. + // + // Registration happens on the worker, after any refresh queued ahead + // of it. A change that such a refresh dispatches does not reach the + // new listener yet, so the replay below hands over the current state + // exactly once. refresher.submit(() -> { - // The listener is already registered, so whatever this refresh - // dispatches reaches it; replay only what it did not. - Refresh dispatched = refresh(); + refresh(); + listeners.add(listener); long deadline = lastDeadline; boolean needsLogin = wasNeedsLogin; handler.post(() -> { - if (!listeners.contains(listener)) { + if (closed || !listeners.contains(listener)) { return; } - if (!dispatched.deadlineChanged) { - notifySafely(() -> listener.onSessionDeadlineChanged(deadline)); - } - if (needsLogin && !dispatched.expired) { + notifySafely(() -> listener.onSessionDeadlineChanged(deadline)); + if (needsLogin) { notifySafely(listener::onSessionExpired); } }); @@ -104,10 +103,13 @@ public boolean isLoginRequired() { } public void removeListener(SessionEventListener listener) { - listeners.remove(listener); + refresher.submit(() -> listeners.remove(listener)); } - private Refresh refresh() { + private void refresh() { + if (closed) { + return; + } long deadline = deadlineUnixSeconds.getAsLong(); boolean deadlineChanged = deadline != lastDeadline; lastDeadline = deadline; @@ -116,12 +118,15 @@ private Refresh refresh() { boolean expired = needsLogin && !wasNeedsLogin; wasNeedsLogin = needsLogin; - Refresh result = new Refresh(deadlineChanged, expired); if (!deadlineChanged && !expired) { - return result; + return; } + List targets = new ArrayList<>(listeners); handler.post(() -> { - for (SessionEventListener l : listeners) { + if (closed) { + return; + } + for (SessionEventListener l : targets) { if (deadlineChanged) { notifySafely(() -> l.onSessionDeadlineChanged(deadline)); } @@ -130,7 +135,6 @@ private Refresh refresh() { } } }); - return result; } private void notifySafely(Runnable r) { From 90c681b3e189e04e99bf1b6288e8d2f126519189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 15 Sep 2026 13:57:29 +0200 Subject: [PATCH 5/7] Read the view binding once before posting from a background thread The worker checked binding for null and then dereferenced the field again; onDestroyView can null it in between and the resulting NPE on a background thread kills the process. Copy the field to a local first. --- .../java/io/netbird/client/ui/home/ExitNodePickerSheet.java | 3 ++- app/src/main/java/io/netbird/client/ui/home/HomeFragment.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java b/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java index 87b07eb8..1ef65ac2 100644 --- a/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java +++ b/app/src/main/java/io/netbird/client/ui/home/ExitNodePickerSheet.java @@ -107,7 +107,8 @@ private void loadExitNodes() { } final boolean selected = anySelected; - View root = binding != null ? binding.getRoot() : null; + SheetExitNodePickerBinding current = binding; + View root = current != null ? current.getRoot() : null; if (root != null) { root.post(() -> showExitNodes(nodes, selected)); } diff --git a/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java b/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java index f24be4d2..d3669598 100644 --- a/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java +++ b/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java @@ -648,7 +648,8 @@ private void runOnUi(Runnable action) { action.run(); return; } - View root = binding != null ? binding.getRoot() : null; + FragmentHomeBinding current = binding; + View root = current != null ? current.getRoot() : null; if (root != null) { root.post(action); } From 14e5e44cacc6ed3d058b3f122c22ca68feacc926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 15 Sep 2026 15:31:50 +0200 Subject: [PATCH 6/7] Resolve the disconnected label when the paint runs, not when it is queued applyEngineState(DISCONNECTED) picked "Disconnected" or "Login required" on the engine callback thread and posted the paint. When the session expires, the engine emits two Disconnected states a few milliseconds before onLoginRequired flips the flag; the queued paints then ran after the direct "Login required" paint and overwrote it, leaving the home screen on "Disconnected" until the next rebind. Read the flag inside the posted paint instead. --- .../netbird/client/ui/home/HomeFragment.java | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java b/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java index d3669598..ad55eeb9 100644 --- a/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java +++ b/app/src/main/java/io/netbird/client/ui/home/HomeFragment.java @@ -24,6 +24,8 @@ import com.google.android.material.switchmaterial.SwitchMaterial; +import java.util.function.IntSupplier; + import io.netbird.client.PlatformUtils; import io.netbird.client.R; import io.netbird.client.ServiceAccessor; @@ -109,7 +111,7 @@ private enum EngineState { CONNECTING, CONNECTED, DISCONNECTING, DISCONNECTED, N // Outlives the engine (and the app process), so it is reported on bind as // well as when it happens — and it overrides the disconnected label, which // on its own would suggest a plain reconnect is enough. - private boolean loginRequired; + private volatile boolean loginRequired; // Keeps the banner's relative text ("in 45 minutes") fresh while visible. private final Runnable sessionTicker = new Runnable() { @Override @@ -399,7 +401,15 @@ private void copyToClipboard(CharSequence value) { } private void setToggle(boolean checked, boolean enabled, int statusResId) { - Log.d(LOGTAG, "UI paint requested: status=" + statusResName(statusResId) + setToggle(checked, enabled, () -> statusResId); + } + + // The label is resolved when the paint runs on the main thread, not when it + // is requested on the engine callback thread: a Disconnected paint queued a + // moment before onLoginRequired flipped the flag must not overwrite + // "Login required". + private void setToggle(boolean checked, boolean enabled, IntSupplier statusRes) { + Log.d(LOGTAG, "UI paint requested: status=" + statusResName(statusRes.getAsInt()) + " toggle=" + checked + " enabled=" + enabled); runOnUi(() -> { if (buttonConnect != null) { @@ -417,7 +427,7 @@ private void setToggle(boolean checked, boolean enabled, int statusResId) { paintEnabledState(enabled); } if (textConnStatus != null) { - textConnStatus.setText(statusResId); + textConnStatus.setText(statusRes.getAsInt()); Log.d(LOGTAG, "UI painted: status=\"" + textConnStatus.getText() + "\""); } }); @@ -530,7 +540,7 @@ private void applyEngineState(EngineState state) { break; case DISCONNECTED: closeForceCancelWindow(); - setToggle(false, true, loginRequired + setToggle(false, true, () -> loginRequired ? R.string.main_status_login_required : R.string.main_status_disconnected); break; From dd289cc88e7e3debe52e5aecffd511f592261af0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Wed, 7 Oct 2026 14:33:01 +0200 Subject: [PATCH 7/7] Stop session callbacks to a listener once it is removed removeListener only queued the removal on the worker, so callbacks that refresh() or the replay had already posted to the main thread still reached the listener after its owner unregistered it. Remove it right away as well, keep the queued removal for an addListener still waiting on the worker, and skip unregistered listeners when dispatching. --- tool/src/main/java/io/netbird/client/tool/SessionMonitor.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java index 4a8406e6..c64dcf39 100644 --- a/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java +++ b/tool/src/main/java/io/netbird/client/tool/SessionMonitor.java @@ -103,6 +103,7 @@ public boolean isLoginRequired() { } public void removeListener(SessionEventListener listener) { + listeners.remove(listener); refresher.submit(() -> listeners.remove(listener)); } @@ -127,6 +128,9 @@ private void refresh() { return; } for (SessionEventListener l : targets) { + if (!listeners.contains(l)) { + continue; + } if (deadlineChanged) { notifySafely(() -> l.onSessionDeadlineChanged(deadline)); }