Prevent deadlocks by avoiding calling callbacks in locked blocks

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/DefaultNetworkMonitor.kt

@@ -25,18 +25,24 @@ object DefaultNetworkMonitor : UpstreamMonitor() {
         override fun onAvailable(network: Network) {
             val properties = app.connectivity.getLinkProperties(network)
             val ifname = properties?.interfaceName ?: return
+            var switching = false
             synchronized(this@DefaultNetworkMonitor) {
                 val oldProperties = currentLinkProperties
                 if (currentNetwork != network || ifname != oldProperties?.interfaceName) {
-                    callbacks.forEach { it.onLost() }   // we are using the other default network now
+                    switching = true    // we are using the other default network now
                     currentNetwork = network
                 }
                 currentLinkProperties = properties
-                callbacks.forEach { it.onAvailable(ifname, properties) }
+                callbacks.toList()
+            }.forEach {
+                if (switching) it.onLost()
+                it.onAvailable(ifname, properties)
             }
         }
 
         override fun onLinkPropertiesChanged(network: Network, properties: LinkProperties) {
+            var losing = true
+            var ifname: String?
             synchronized(this@DefaultNetworkMonitor) {
                 if (currentNetwork == null) {
                     onAvailable(network)
@@ -45,30 +51,28 @@ object DefaultNetworkMonitor : UpstreamMonitor() {
                 if (currentNetwork != network) return
                 val oldProperties = currentLinkProperties!!
                 currentLinkProperties = properties
-                val ifname = properties.interfaceName
-                when {
-                    ifname == null -> {
-                        Timber.w("interfaceName became null: $oldProperties -> $properties")
-                        onLost(network)
-                    }
-                    ifname != oldProperties.interfaceName -> {
-                        Timber.w(RuntimeException("interfaceName changed: $oldProperties -> $properties"))
-                        callbacks.forEach {
-                            it.onLost()
-                            it.onAvailable(ifname, properties)
-                        }
-                    }
-                    else -> callbacks.forEach { it.onAvailable(ifname, properties) }
+                ifname = properties.interfaceName
+                when (ifname) {
+                    null -> Timber.w("interfaceName became null: $oldProperties -> $properties")
+                    oldProperties.interfaceName -> losing = false
+                    else -> Timber.w(RuntimeException("interfaceName changed: $oldProperties -> $properties"))
                 }
+                callbacks.toList()
+            }.forEach {
+                if (losing) {
+                    if (ifname == null) return onLost(network)
+                    it.onLost()
+                }
+                ifname?.let { ifname -> it.onAvailable(ifname, properties) }
             }
         }
 
         override fun onLost(network: Network) = synchronized(this@DefaultNetworkMonitor) {
             if (currentNetwork != network) return
-            callbacks.forEach { it.onLost() }
             currentNetwork = null
             currentLinkProperties = null
-        }
+            callbacks.toList()
+        }.forEach { it.onLost() }
     }
 
     override fun registerCallbackLocked(callback: Callback) {

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/FallbackUpstreamMonitor.kt

@@ -27,10 +27,10 @@ abstract class FallbackUpstreamMonitor private constructor() : UpstreamMonitor()
                 synchronized(this) {
                     val old = monitor
                     val callbacks = synchronized(old) {
-                        val callbacks = old.callbacks.toList()
-                        old.callbacks.clear()
-                        old.destroyLocked()
-                        callbacks
+                        old.callbacks.toList().also {
+                            old.callbacks.clear()
+                            old.destroyLocked()
+                        }
                     }
                     val new = generateMonitor()
                     monitor = new

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/InterfaceMonitor.kt

@@ -1,21 +1,27 @@
 package be.mygod.vpnhotspot.net.monitor
 
+import android.net.LinkProperties
 import be.mygod.vpnhotspot.App.Companion.app
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.launch
 
 class InterfaceMonitor(val iface: String) : UpstreamMonitor() {
-    private fun setPresent(present: Boolean) = synchronized(this) {
-        val old = currentIface != null
-        if (present == old) return
-        currentIface = if (present) iface else null
-        if (present) Pair(iface, currentLinkProperties) else null
-    }.let { pair ->
-        if (pair != null) {
-            val (iface, lp) = pair
-            lp ?: return
-            callbacks.forEach { it.onAvailable(iface, lp) }
-        } else callbacks.forEach { it.onLost() }
+    private fun setPresent(present: Boolean) {
+        var available: Pair<String, LinkProperties>? = null
+        synchronized(this) {
+            val old = currentIface != null
+            if (present == old) return
+            currentIface = if (present) iface else null
+            if (present) available = iface to (currentLinkProperties ?: return)
+            callbacks.toList()
+        }.forEach {
+            @Suppress("NAME_SHADOWING")
+            val available = available
+            if (available != null) {
+                val (iface, lp) = available
+                it.onAvailable(iface, lp)
+            } else it.onLost()
+        }
     }
 
     private var registered = false

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/UpstreamMonitor.kt

@@ -8,8 +8,6 @@ import android.os.Build
 import be.mygod.vpnhotspot.App.Companion.app
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.launch
-import java.util.*
-import java.util.concurrent.ConcurrentHashMap
 
 abstract class UpstreamMonitor {
     companion object : SharedPreferences.OnSharedPreferenceChangeListener {
@@ -40,11 +38,10 @@ abstract class UpstreamMonitor {
                 synchronized(this) {
                     val old = monitor
                     val (active, callbacks) = synchronized(old) {
-                        val active = old.currentIface != null
-                        val callbacks = old.callbacks.toList()
-                        old.callbacks.clear()
-                        old.destroyLocked()
-                        Pair(active, callbacks)
+                        (old.currentIface != null) to old.callbacks.toList().also {
+                            old.callbacks.clear()
+                            old.destroyLocked()
+                        }
                     }
                     val new = generateMonitor()
                     monitor = new
@@ -78,7 +75,7 @@ abstract class UpstreamMonitor {
         }
     }
 
-    val callbacks = Collections.newSetFromMap(ConcurrentHashMap<Callback, Boolean>())
+    val callbacks = mutableSetOf<Callback>()
     protected abstract val currentLinkProperties: LinkProperties?
     open val currentIface: String? get() = currentLinkProperties?.interfaceName
     protected abstract fun registerCallbackLocked(callback: Callback)

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/VpnMonitor.kt

@@ -23,17 +23,23 @@ object VpnMonitor : UpstreamMonitor() {
         override fun onAvailable(network: Network) {
             val properties = app.connectivity.getLinkProperties(network)
             val ifname = properties?.interfaceName ?: return
+            var switching = false
             synchronized(this@VpnMonitor) {
                 val oldProperties = available.put(network, properties)
                 if (currentNetwork != network || ifname != oldProperties?.interfaceName) {
-                    if (currentNetwork != null) callbacks.forEach { it.onLost() }
+                    if (currentNetwork != null) switching = true
                     currentNetwork = network
                 }
-                callbacks.forEach { it.onAvailable(ifname, properties) }
+                callbacks.toList()
+            }.forEach {
+                if (switching) it.onLost()
+                it.onAvailable(ifname, properties)
             }
         }
 
         override fun onLinkPropertiesChanged(network: Network, properties: LinkProperties) {
+            var losing = true
+            var ifname: String?
             synchronized(this@VpnMonitor) {
                 if (currentNetwork == null) {
                     onAvailable(network)
@@ -41,33 +47,37 @@ object VpnMonitor : UpstreamMonitor() {
                 }
                 if (currentNetwork != network) return
                 val oldProperties = available.put(network, properties)!!
-                val ifname = properties.interfaceName
-                when {
-                    ifname == null -> {
-                        Timber.w("interfaceName became null: $oldProperties -> $properties")
-                        onLost(network)
-                    }
-                    ifname != oldProperties.interfaceName -> {
-                        Timber.w("interfaceName changed: $oldProperties -> $properties")
-                        callbacks.forEach {
-                            it.onLost()
-                            it.onAvailable(ifname, properties)
-                        }
-                    }
-                    else -> callbacks.forEach { it.onAvailable(ifname, properties) }
+                ifname = properties.interfaceName
+                when (ifname) {
+                    null -> Timber.w("interfaceName became null: $oldProperties -> $properties")
+                    oldProperties.interfaceName -> losing = false
+                    else -> Timber.w(RuntimeException("interfaceName changed: $oldProperties -> $properties"))
                 }
+                callbacks.toList()
+            }.forEach {
+                if (losing) {
+                    if (ifname == null) return onLost(network)
+                    it.onLost()
+                }
+                ifname?.let { ifname -> it.onAvailable(ifname, properties) }
             }
         }
 
-        override fun onLost(network: Network) = synchronized(this@VpnMonitor) {
-            if (available.remove(network) == null || currentNetwork != network) return
-            callbacks.forEach { it.onLost() }
-            if (available.isNotEmpty()) {
-                val next = available.entries.first()
-                currentNetwork = next.key
-                Timber.d("Switching to ${next.value.interfaceName} as VPN interface")
-                callbacks.forEach { it.onAvailable(next.value.interfaceName!!, next.value) }
-            } else currentNetwork = null
+        override fun onLost(network: Network) {
+            var newProperties: LinkProperties? = null
+            synchronized(this@VpnMonitor) {
+                if (available.remove(network) == null || currentNetwork != network) return
+                if (available.isNotEmpty()) {
+                    val next = available.entries.first()
+                    currentNetwork = next.key
+                    Timber.d("Switching to ${next.value.interfaceName} as VPN interface")
+                    newProperties = next.value
+                } else currentNetwork = null
+                callbacks.toList()
+            }.forEach {
+                it.onLost()
+                newProperties?.let { prop -> it.onAvailable(prop.interfaceName!!, prop) }
+            }
         }
     }