Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Make ColibriSession.relayId non-nullable (avoid use of !!). #1110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import kotlin.collections.HashSet
* A representation of a cascade of bridges.
*/
interface Cascade<N : CascadeNode<N, L>, L : CascadeLink> {
val sessions: MutableMap<String?, N>
val sessions: MutableMap<String, N>
fun addLinkBetween(session: N, otherSession: N, meshId: String)
fun removeLinkTo(session: N, otherSession: N)
}
Expand All @@ -33,15 +33,15 @@ interface Cascade<N : CascadeNode<N, L>, L : CascadeLink> {
* A representation of a single bridge in a cascade
*/
interface CascadeNode<N : CascadeNode<N, L>, L : CascadeLink> {
val relayId: String?
val relayId: String
val relays: MutableMap<String, L>
}

/**
* A representation of a link between bridges in a cascade
*/
interface CascadeLink {
val relayId: String?
val relayId: String
val meshId: String?
}

Expand Down Expand Up @@ -139,7 +139,7 @@ fun <C : Cascade<N, L>, N : CascadeNode<N, L>, 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) ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class ColibriV2SessionManager(
/**
* The colibri2 sessions that are currently active, mapped by the [Bridge] that they use.
*/
override val sessions = mutableMapOf<String?, Colibri2Session>()
override val sessions = mutableMapOf<String, Colibri2Session>()

/**
* The set of participants that have associated colibri2 endpoints allocated, mapped by their ID. A participant is
Expand Down Expand Up @@ -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()
}

Expand Down Expand Up @@ -231,7 +229,7 @@ class ColibriV2SessionManager(
*/
private fun getOrCreateSession(bridge: Bridge, visitor: Boolean):
Pair<Colibri2Session, Boolean> = synchronized(syncRoot) {
var session = sessions[bridge.relayId]
var session = sessions[bridge.jid.toString()]
if (session != null) {
return Pair(session, false)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -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."
Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -620,12 +616,13 @@ 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.") }
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder
import io.kotest.matchers.shouldBe

class TestCascade : Cascade<TestCascadeNode, TestCascadeLink> {
override val sessions = HashMap<String?, TestCascadeNode>()
override val sessions = HashMap<String, TestCascadeNode>()

var linksRemoved = 0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ private fun mockBridge(r: Regions, s: StressLevels) = mockk<Bridge> {
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]"
}

Expand Down
Loading