Skip to content

Commit

Permalink
feat: Remove support for ICE/TCP.
Browse files Browse the repository at this point in the history
* TURN/TLS provides a better solution for the same problem.
* The implementation has been known to have issues with threads getting blocked.
* ICE/TCP gets in the way of improvements we're currently working on.
* We (jitsi team) haven't used it in any of our deployments since at least 2018.
* See also: jitsi/docker-jitsi-meet@7a93978
  • Loading branch information
bgrozev committed Jul 18, 2024
1 parent 877b6a2 commit 55477be
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 103 deletions.
27 changes: 2 additions & 25 deletions jvb/src/main/kotlin/org/jitsi/videobridge/ice/Harvesters.kt
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,15 @@
package org.jitsi.videobridge.ice

import org.ice4j.ice.harvest.SinglePortUdpHarvester
import org.ice4j.ice.harvest.TcpHarvester
import org.jitsi.utils.logging2.createLogger
import java.io.IOException

class Harvesters private constructor(
val tcpHarvester: TcpHarvester?,
val singlePortHarvesters: List<SinglePortUdpHarvester>
) {
class Harvesters private constructor(val singlePortHarvesters: List<SinglePortUdpHarvester>) {
/* We're unhealthy if there are no single port harvesters. */
val healthy: Boolean
get() = singlePortHarvesters.isNotEmpty()

private fun close() {
singlePortHarvesters.forEach { it.close() }
tcpHarvester?.close()
}

companion object {
Expand All @@ -48,25 +42,8 @@ class Harvesters private constructor(
if (singlePortHarvesters.isEmpty()) {
logger.warn("No single-port harvesters created.")
}
val tcpHarvester: TcpHarvester? = if (IceConfig.config.tcpEnabled) {
val port = IceConfig.config.tcpPort
try {
TcpHarvester(port, IceConfig.config.iceSslTcp).apply {
logger.info("Initialized TCP harvester on port $port, ssltcp=${IceConfig.config.iceSslTcp}")
IceConfig.config.tcpMappedPort?.let { mappedPort ->
logger.info("Adding mapped port $mappedPort")
addMappedPort(mappedPort)
}
}
} catch (ioe: IOException) {
logger.warn("Failed to initialize TCP harvester on port $port")
null
}
} else {
null
}

Harvesters(tcpHarvester, singlePortHarvesters)
Harvesters(singlePortHarvesters)
}
}
}
34 changes: 0 additions & 34 deletions jvb/src/main/kotlin/org/jitsi/videobridge/ice/IceConfig.kt
Original file line number Diff line number Diff line change
Expand Up @@ -24,40 +24,6 @@ import org.jitsi.metaconfig.from
import org.jitsi.metaconfig.optionalconfig

class IceConfig private constructor() {
/**
* Is ICE/TCP enabled.
*/
val tcpEnabled: Boolean by config {
// The old property is named 'disable', while the new one
// is 'enable', so invert the old value
"org.jitsi.videobridge.DISABLE_TCP_HARVESTER".from(JitsiConfig.legacyConfig).transformedBy { !it }
"videobridge.ice.tcp.enabled".from(JitsiConfig.newConfig)
}

/**
* The ICE/TCP port.
*/
val tcpPort: Int by config {
"org.jitsi.videobridge.TCP_HARVESTER_PORT".from(JitsiConfig.legacyConfig)
"videobridge.ice.tcp.port".from(JitsiConfig.newConfig)
}

/**
* The additional port to advertise, or null if none is configured.
*/
val tcpMappedPort: Int? by optionalconfig {
"org.jitsi.videobridge.TCP_HARVESTER_MAPPED_PORT".from(JitsiConfig.legacyConfig)
"videobridge.ice.tcp.mapped-port".from(JitsiConfig.newConfig)
}

/**
* Whether ICE/TCP should use "ssltcp" or not.
*/
val iceSslTcp: Boolean by config {
"org.jitsi.videobridge.TCP_HARVESTER_SSLTCP".from(JitsiConfig.legacyConfig)
"videobridge.ice.tcp.ssltcp".from(JitsiConfig.newConfig)
}

/**
* The ICE UDP port.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_DATA_CHANNE
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_DOMINANT_SPEAKER_CHANGES
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_ICE_FAILED
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_ICE_SUCCEEDED
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_ICE_SUCCEEDED_TCP
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_PACKETS_RECEIVED
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_PACKETS_RECEIVED_OCTO
import org.jitsi.xmpp.extensions.colibri.ColibriStatsExtension.TOTAL_PACKETS_SENT
Expand Down Expand Up @@ -199,7 +198,6 @@ object VideobridgeStatisticsShim {

put(TOTAL_ICE_FAILED, IceTransport.iceFailed.get())
put(TOTAL_ICE_SUCCEEDED, IceTransport.iceSucceeded.get())
put(TOTAL_ICE_SUCCEEDED_TCP, IceTransport.iceSucceededTcp.get())
put("total_ice_succeeded_relayed", IceTransport.iceSucceededRelayed.get())

put("average_participant_stress", JvbLoadManager.averageParticipantStress)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,6 @@ class IceTransport @JvmOverloads constructor(
transition.completed() -> {
if (iceConnected.compareAndSet(false, true)) {
eventHandler?.connected()
if (iceComponent.selectedPair.remoteCandidate.transport.isTcpType()) {
iceSucceededTcp.inc()
}
if (iceComponent.selectedPair.remoteCandidate.type == CandidateType.RELAYED_CANDIDATE ||
iceComponent.selectedPair.localCandidate.type == CandidateType.RELAYED_CANDIDATE
) {
Expand Down Expand Up @@ -393,9 +390,6 @@ class IceTransport @JvmOverloads constructor(

companion object {
fun appendHarvesters(iceAgent: Agent) {
Harvesters.INSTANCE.tcpHarvester?.let {
iceAgent.addCandidateHarvester(it)
}
Harvesters.INSTANCE.singlePortHarvesters.forEach(iceAgent::addCandidateHarvester)
}

Expand All @@ -416,15 +410,6 @@ class IceTransport @JvmOverloads constructor(
"Number of times an ICE Agent succeeded."
)

/**
* The total number of times an ICE Agent succeeded and the selected
* candidate was a TCP candidate.
*/
val iceSucceededTcp = VideobridgeMetricsContainer.instance.registerCounter(
"ice_succeeded_tcp",
"Number of times an ICE Agent succeeded and the selected candidate was a TCP candidate."
)

/**
* The total number of times an ICE Agent succeeded and the selected
* candidate pair included a relayed candidate.
Expand Down Expand Up @@ -502,8 +487,6 @@ private fun TransportAddress.isPrivateAddress(): Boolean = address.isSiteLocalAd
/* 0xfc00::/7 */
((address is Inet6Address) && ((addressBytes[0].toInt() and 0xfe) == 0xfc))

private fun Transport.isTcpType(): Boolean = this == Transport.TCP || this == Transport.SSLTCP

private fun generateCandidateId(candidate: LocalCandidate): String = buildString {
append(java.lang.Long.toHexString(hashCode().toLong()))
append(java.lang.Long.toHexString(candidate.parentComponent.parentStream.parentAgent.hashCode().toLong()))
Expand All @@ -526,16 +509,7 @@ private fun LocalCandidate.toCandidatePacketExtension(advertisePrivateAddresses:
cpe.network = 0
cpe.setPriority(priority)

// Advertise 'tcp' candidates for which SSL is enabled as 'ssltcp'
// (although internally their transport protocol remains "tcp")
cpe.protocol = if (transport == Transport.TCP && isSSL) {
Transport.SSLTCP.toString()
} else {
transport.toString()
}
if (transport.isTcpType()) {
cpe.tcpType = tcpType.toString()
}
cpe.protocol = transport.toString()
cpe.type = org.jitsi.xmpp.extensions.jingle.CandidateType.valueOf(type.toString())
cpe.ip = transportAddress.hostAddress
cpe.port = transportAddress.port
Expand Down
17 changes: 2 additions & 15 deletions jvb/src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -264,19 +264,6 @@ videobridge {
relay-domains = []
}
ice {
tcp {
# Whether ICE/TCP is enabled.
enabled = false

# The port to bind to for ICE/TCP.
port = 443

# An optional additional port to advertise.
# mapped-port = 8443
# Whether to use "ssltcp" or plain "tcp".
ssltcp = true
}

udp {
# The port for ICE/UDP.
port = 10000
Expand All @@ -286,8 +273,8 @@ videobridge {
#ufrag-prefix = "jvb-123:"

# Which candidate pairs to keep alive. The accepted values are defined in ice4j's KeepAliveStrategy:
# "selected_and_tcp", "selected_only", or "all_succeeded".
keep-alive-strategy = "selected_and_tcp"
# "selected_only", or "all_succeeded".
keep-alive-strategy = "selected_only"

# Whether to use the "component socket" feature of ice4j.
use-component-socket = true
Expand Down

0 comments on commit 55477be

Please sign in to comment.