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..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 @@ -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,18 @@ private void loadExitNodes() { } } + final boolean selected = anySelected; + SheetExitNodePickerBinding current = binding; + View root = current != null ? current.getRoot() : null; + if (root != null) { + root.post(() -> showExitNodes(nodes, selected)); + } + } + + 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..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,12 +24,15 @@ 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; 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 +45,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 +54,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,11 +104,14 @@ 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 // 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 @@ -239,9 +248,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); @@ -394,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) { @@ -412,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() + "\""); } }); @@ -525,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; @@ -643,7 +658,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); } @@ -683,6 +699,12 @@ public void onDetach() { serviceAccessor = null; } + @Override + public void onDestroy() { + engineQueries.shutdown(); + super.onDestroy(); + } + @Override public void onEngineStarted() { @@ -705,6 +727,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 +864,7 @@ public void onPeersListChanged(long numberOfPeers) { @Override public void onSessionDeadlineChanged(long expiresAtUnixSeconds) { + sessionDeadlineVersion++; sessionDeadlineUnixSeconds = expiresAtUnixSeconds; runOnUi(this::updateSessionRow); } @@ -853,6 +880,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..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 @@ -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,13 @@ 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); + // 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<>())); @@ -48,6 +56,7 @@ public void setServiceAccessor(@Nullable ServiceAccessor accessor) { @Override protected void onCleared() { super.onCleared(); + resourcesRefresher.shutdown(); setServiceAccessor(null); } @@ -138,6 +147,14 @@ private List getRoutingPeers(PeerInfoArray peersFromEngine) { } private void postResources() { + resourcesRefresher.request(); + } + + private void loadResources() { + int generation; + synchronized (publishLock) { + generation = resourcesGeneration; + } ServiceAccessor accessor = serviceAccessor; if (accessor == null) { return; @@ -152,8 +169,14 @@ private void postResources() { return; } - // This value will be set from a background thread. - 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 @@ -162,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/CoalescingWorker.java b/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java new file mode 100644 index 00000000..0e57f6c0 --- /dev/null +++ b/tool/src/main/java/io/netbird/client/tool/CoalescingWorker.java @@ -0,0 +1,59 @@ +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 + } + } + + /** Stops the worker. Queued runs are dropped; a run in progress finishes. */ + public void shutdown() { + executor.shutdownNow(); + } + + 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 aaba0f02..f444165e 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; @@ -82,6 +83,7 @@ public String status() { /** Session deadline as unix seconds, or 0 when none is known. */ public long sessionExpiresAt() { + warnIfOnMainThread("sessionExpiresAt"); return goClient.sessionExpiresAtUnix(); } @@ -342,11 +344,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"); @@ -370,6 +374,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 df0f3b65..745824b1 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; @@ -15,8 +17,9 @@ * NeedsLogin status label; the expiry warnings themselves are scheduled from * the deadline by {@link SessionWarningScheduler}. * - *

All state lives on the main thread; {@link #onStateChanged()} 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()} may be called from any thread. */ public class SessionMonitor { @@ -29,6 +32,8 @@ 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 volatile boolean closed; private long lastDeadline; private boolean wasNeedsLogin; @@ -38,25 +43,44 @@ public SessionMonitor(Supplier status, LongSupplier deadlineUnixSeconds) this.deadlineUnixSeconds = deadlineUnixSeconds; } + /** Stops the worker and drops every listener. Call when the owning service is destroyed. */ + public void shutdown() { + closed = true; + listeners.clear(); + refresher.shutdown(); + } + /** Wake-up from the Go state-change signal. Safe to call from any thread. */ public void onStateChanged() { - handler.post(this::refresh); + refresher.request(); } 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. - handler.post(() -> { + // + // 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(() -> { refresh(); - notifySafely(() -> listener.onSessionDeadlineChanged(lastDeadline)); - if (isLoginRequired()) { - notifySafely(listener::onSessionExpired); - } + listeners.add(listener); + long deadline = lastDeadline; + boolean needsLogin = wasNeedsLogin; + handler.post(() -> { + if (closed || !listeners.contains(listener)) { + return; + } + notifySafely(() -> listener.onSessionDeadlineChanged(deadline)); + if (needsLogin) { + notifySafely(listener::onSessionExpired); + } + }); }); } @@ -67,24 +91,41 @@ public boolean isLoginRequired() { public void removeListener(SessionEventListener listener) { listeners.remove(listener); + refresher.submit(() -> listeners.remove(listener)); } private void refresh() { - long deadline = deadlineUnixSeconds.getAsLong(); - if (deadline != lastDeadline) { - lastDeadline = deadline; - for (SessionEventListener l : listeners) { - notifySafely(() -> l.onSessionDeadlineChanged(deadline)); - } + if (closed) { + return; } + long deadline = deadlineUnixSeconds.getAsLong(); + boolean deadlineChanged = deadline != lastDeadline; + lastDeadline = deadline; boolean needsLogin = STATUS_NEEDS_LOGIN.equals(status.get()); - if (needsLogin && !wasNeedsLogin) { - for (SessionEventListener l : listeners) { - notifySafely(l::onSessionExpired); - } - } + boolean expired = needsLogin && !wasNeedsLogin; wasNeedsLogin = needsLogin; + + if (!deadlineChanged && !expired) { + return; + } + List targets = new ArrayList<>(listeners); + handler.post(() -> { + if (closed) { + return; + } + for (SessionEventListener l : targets) { + if (!listeners.contains(l)) { + continue; + } + if (deadlineChanged) { + notifySafely(() -> l.onSessionDeadlineChanged(deadline)); + } + if (expired) { + notifySafely(l::onSessionExpired); + } + } + }); } 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 9bc1d954..cb38d2b1 100644 --- a/tool/src/main/java/io/netbird/client/tool/VPNService.java +++ b/tool/src/main/java/io/netbird/client/tool/VPNService.java @@ -206,6 +206,7 @@ public void onDestroy() { networkChangeDetector.unsubscribe(); networkChangeDetector.unregisterNetworkCallback(); + sessionMonitor.shutdown(); engineRunner.stop(); stopForeground(true); 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(); + } + } +}