From 5d5c01e4be4283ec77ad40d56def789e012475c9 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Wed, 16 Aug 2023 16:53:38 -0500 Subject: [PATCH 1/2] ref: Make ColibriSession.relayId non-nullable (avoid use of !!). --- .../kotlin/org/jitsi/jicofo/bridge/Bridge.kt | 6 ++-- .../jicofo/bridge/BridgeSelectionStrategy.kt | 2 +- .../kotlin/org/jitsi/jicofo/bridge/Cascade.kt | 8 ++--- .../jicofo/bridge/colibri/Colibri2Session.kt | 2 +- .../bridge/colibri/ColibriV2SessionManager.kt | 30 +++++++++---------- .../org/jitsi/jicofo/bridge/CascadeTest.kt | 2 +- .../jicofo/bridge/RegionBasedSelectionTest.kt | 2 +- 7 files changed, 26 insertions(+), 26 deletions(-) diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Bridge.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Bridge.kt index ea0f07afd0..6244a31d13 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Bridge.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Bridge.kt @@ -149,8 +149,10 @@ class Bridge @JvmOverloads internal constructor( * @return the relay ID advertised by the bridge, or `null` if * none was advertised. */ - var relayId: String? = null - private set + private var relayId: String? = null + + val supportsRelay + get() = relayId != null private val logger: Logger = LoggerImpl(Bridge::class.java.name) diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/BridgeSelectionStrategy.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/BridgeSelectionStrategy.kt index 255bbae720..5a034fa592 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/BridgeSelectionStrategy.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/BridgeSelectionStrategy.kt @@ -123,7 +123,7 @@ abstract class BridgeSelectionStrategy { bridge } else { val existingBridge = conferenceBridges.keys.first() - if (!allowMultiBridge || existingBridge.relayId == null) { + if (!allowMultiBridge || !existingBridge.supportsRelay) { logger.info("Existing bridge does not have a relay, will not consider other bridges.") return existingBridge } diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Cascade.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Cascade.kt index 57f0fb0625..e54bd4b2fd 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Cascade.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/Cascade.kt @@ -24,7 +24,7 @@ import kotlin.collections.HashSet * A representation of a cascade of bridges. */ interface Cascade, L : CascadeLink> { - val sessions: MutableMap + val sessions: MutableMap fun addLinkBetween(session: N, otherSession: N, meshId: String) fun removeLinkTo(session: N, otherSession: N) } @@ -33,7 +33,7 @@ interface Cascade, L : CascadeLink> { * A representation of a single bridge in a cascade */ interface CascadeNode, L : CascadeLink> { - val relayId: String? + val relayId: String val relays: MutableMap } @@ -41,7 +41,7 @@ interface CascadeNode, L : CascadeLink> { * A representation of a link between bridges in a cascade */ interface CascadeLink { - val relayId: String? + val relayId: String val meshId: String? } @@ -139,7 +139,7 @@ fun , N : CascadeNode, L : CascadeLink> C.removeNode( if (meshes.size > 1) { /* The removed node was a bridge between two or more meshes - we need to repair the cascade. */ val disconnected = node.relays.values.groupBy { it.meshId }.values.map { - it.flatMap { getNodesBehind(node, it.relayId!!) }.toSet() + it.flatMap { getNodesBehind(node, it.relayId) }.toSet() }.toSet() val newLinks = repairFn(this, disconnected) newLinks.forEach { (node, other, mesh) -> diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt index 9c96d582db..133cf5d179 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt @@ -69,7 +69,7 @@ class Colibri2Session( * change in the context of a session. We maintain the invariant that whenever a conference has multiple sessions, * they all have non-null relay IDs. */ - override val relayId: String? = bridge.relayId + override val relayId: String = bridge.jid.toString() /** * Whether the colibri2 conference has been created. It is created with the first endpoint allocation request diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt index 8d462d1cde..38172fbad4 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt @@ -83,7 +83,7 @@ class ColibriV2SessionManager( /** * The colibri2 sessions that are currently active, mapped by the [Bridge] that they use. */ - override val sessions = mutableMapOf() + override val sessions = mutableMapOf() /** * The set of participants that have associated colibri2 endpoints allocated, mapped by their ID. A participant is @@ -136,9 +136,7 @@ class ColibriV2SessionManager( sessions.remove(session.relayId) participantsBySession.remove(session) participants.forEach { remove(it) } - session.relayId?.let { removedRelayId -> - sessions.values.forEach { otherSession -> otherSession.expireRelay(removedRelayId) } - } + sessions.values.forEach { otherSession -> otherSession.expireRelay(session.relayId) } return participants.toSet() } @@ -231,7 +229,7 @@ class ColibriV2SessionManager( */ private fun getOrCreateSession(bridge: Bridge, visitor: Boolean): Pair = synchronized(syncRoot) { - var session = sessions[bridge.relayId] + var session = sessions[bridge.jid.toString()] if (session != null) { return Pair(session, false) } @@ -261,14 +259,12 @@ class ColibriV2SessionManager( getVisibleSessionParticipants(it) } - session.createRelay(otherSession.relayId!!, participantsBehindOtherSession, initiator = true, meshId) - otherSession.createRelay(session.relayId!!, participantsBehindSession, initiator = false, meshId) + session.createRelay(otherSession.relayId, participantsBehindOtherSession, initiator = true, meshId) + otherSession.createRelay(session.relayId, participantsBehindSession, initiator = false, meshId) } override fun removeLinkTo(session: Colibri2Session, otherSession: Colibri2Session) { - otherSession.relayId?.let { removedRelayId -> - session.expireRelay(removedRelayId) - } + session.expireRelay(otherSession.relayId) } @Throws(ColibriAllocationFailedException::class, BridgeSelectionFailedException::class) @@ -308,7 +304,7 @@ class ColibriV2SessionManager( // This is a bridge selection failure, because the selector should not have returned a different // bridge when Octo is not enabled. throw BridgeSelectionFailedException() - } else if (sessions.any { it.value.relayId == null } || bridge.relayId == null) { + } else if (sessions.any { !it.value.bridge.supportsRelay } || !bridge.supportsRelay) { logger.error("Can not enable Octo: one of the selected bridges does not support Octo.") // This is a bridge selection failure, because the selector should not have returned a different // bridge when one of the bridges doesn't support Octo (does not have a relay ID). @@ -344,7 +340,7 @@ class ColibriV2SessionManager( "from ${from.relayId}." } // We already made sure that relayId is not null when there are multiple sessions. - otherSession.updateRemoteParticipant(participantInfo, from.relayId!!, create = true) + otherSession.updateRemoteParticipant(participantInfo, from.relayId, create = true) } } } @@ -413,7 +409,7 @@ class ColibriV2SessionManager( // * Do nothing (if this is due to an internal error we don't want to retry indefinitely) // * Re-invite the participants (possibly on the same bridge) on this bridge // * Re-invite the participants on this bridge to a different bridge - if (!sessions.containsKey(session.bridge.relayId)) { + if (!sessions.containsKey(session.bridge.jid.toString())) { val reinvite = participants[participantInfo.id] == null logger.warn( "Response for an unknown session, will ${if (reinvite) "" else "not "} reinvite the participant." @@ -563,7 +559,7 @@ class ColibriV2SessionManager( getPathsFrom(participantInfo.session) { _, otherSession, from -> if (from != null) { // We make sure that relayId is not null when there are multiple sessions. - otherSession.updateRemoteParticipant(participantInfo, from.relayId!!, false) + otherSession.updateRemoteParticipant(participantInfo, from.relayId, false) } } } @@ -620,12 +616,14 @@ class ColibriV2SessionManager( logger.debug { "Received transport from $session for relay $relayId: ${transport.toXML()}" } synchronized(syncRoot) { // It's possible a new session was started for the same bridge. - if (!sessions.containsKey(session.bridge.relayId) || sessions[session.bridge.relayId] != session) { + if (!sessions.containsKey(session.bridge.jid.toString()) || + sessions[session.bridge.jid.toString()] != session + ) { logger.info("Received a response for a session that is no longer active. Ignoring.") return } // We make sure relayId is not null when there are multiple sessions. - sessions.values.find { it.relayId == relayId }?.setRelayTransport(transport, session.relayId!!) + sessions.values.find { it.relayId == relayId }?.setRelayTransport(transport, session.relayId) ?: { logger.warn("Response for a relay that is no longer active. Ignoring.") } } } diff --git a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/CascadeTest.kt b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/CascadeTest.kt index 4c8ba09550..49b5a61ee3 100644 --- a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/CascadeTest.kt +++ b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/CascadeTest.kt @@ -26,7 +26,7 @@ import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder import io.kotest.matchers.shouldBe class TestCascade : Cascade { - override val sessions = HashMap() + override val sessions = HashMap() var linksRemoved = 0 diff --git a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/RegionBasedSelectionTest.kt b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/RegionBasedSelectionTest.kt index d59539e630..5feca57946 100644 --- a/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/RegionBasedSelectionTest.kt +++ b/jicofo-selector/src/test/kotlin/org/jitsi/jicofo/bridge/RegionBasedSelectionTest.kt @@ -178,7 +178,7 @@ private fun mockBridge(r: Regions, s: StressLevels) = mockk { every { stress } returns s.stress every { isOverloaded } returns (s == High) every { lastReportedStressLevel } returns s.stress - every { relayId } returns "dummy" + every { supportsRelay } returns true every { this@mockk.toString() } returns "MockBridge[region=$region, stress=$stress]" } From 9eaae8363f4d722de68efc7d7db0f46c97328877 Mon Sep 17 00:00:00 2001 From: Boris Grozev Date: Mon, 21 Aug 2023 14:59:42 -0500 Subject: [PATCH 2/2] squash: Remove leftover comment --- .../org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt index 38172fbad4..3a563c5bfd 100644 --- a/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt +++ b/jicofo-selector/src/main/kotlin/org/jitsi/jicofo/bridge/colibri/ColibriV2SessionManager.kt @@ -622,7 +622,6 @@ class ColibriV2SessionManager( logger.info("Received a response for a session that is no longer active. Ignoring.") return } - // We make sure relayId is not null when there are multiple sessions. sessions.values.find { it.relayId == relayId }?.setRelayTransport(transport, session.relayId) ?: { logger.warn("Response for a relay that is no longer active. Ignoring.") } }