diff --git a/src/main/java/org/ice4j/ice/Agent.java b/src/main/java/org/ice4j/ice/Agent.java
index 29236a3d..67f25fc8 100644
--- a/src/main/java/org/ice4j/ice/Agent.java
+++ b/src/main/java/org/ice4j/ice/Agent.java
@@ -242,7 +242,7 @@ public void run()
private long tieBreaker;
/**
- * Determines whether this is the controlling agent in a an ICE interaction.
+ * Determines whether this agent has a controlling role in an ICE interaction.
*/
private boolean isControlling = true;
@@ -402,7 +402,8 @@ public Agent(String ufragPrefix, Logger parentLogger)
addCandidateHarvester(harvester);
}
- logger.debug(() -> "Created a new Agent");
+ logger.debug(() -> "Created a new Agent: " + this.toString() +
+ " with ICE controlling role = " + this.isControlling);
}
/**
@@ -1407,6 +1408,13 @@ public long getTieBreaker()
*/
public void setControlling(boolean isControlling)
{
+ if (this.isControlling != isControlling)
+ {
+ this.logger.info(() -> "Changing agent " + this.toString() +
+ " role from controlling = " + this.isControlling +
+ " to controlling = " + isControlling);
+ }
+
this.isControlling = isControlling;
//in case we have already initialized our check lists we'd need to
diff --git a/src/main/java/org/ice4j/ice/ConnectivityCheckServer.java b/src/main/java/org/ice4j/ice/ConnectivityCheckServer.java
index a094a85c..c6390573 100644
--- a/src/main/java/org/ice4j/ice/ConnectivityCheckServer.java
+++ b/src/main/java/org/ice4j/ice/ConnectivityCheckServer.java
@@ -35,23 +35,6 @@ class ConnectivityCheckServer
implements RequestListener,
CredentialsAuthority
{
- /**
- * Compares a and b as unsigned long values. Serves the
- * same purpose as the Long.compareUnsigned method available in
- * Java 1.8.
- * @return -1 if a is less than b, 0 if
- * they are equal and 1 if a is bigger.
- */
- private static int compareUnsignedLong(long a, long b)
- {
- if (a == b)
- return 0;
- else if ((a + Long.MIN_VALUE) < (b + Long.MIN_VALUE))
- return -1;
- else
- return 1;
- }
-
/**
* The agent that created us.
*/
@@ -259,107 +242,78 @@ private long extractPriority(Request request)
*/
private boolean repairRoleConflict(StunMessageEvent evt)
{
- Message req = evt.getMessage();
- long ourTieBreaker = parentAgent.getTieBreaker();
-
+ final Message req = evt.getMessage();
+ final boolean selfIceControlling = parentAgent.isControlling();
// If the agent is in the controlling role, and the
// ICE-CONTROLLING attribute is present in the request:
- if(parentAgent.isControlling()
- && req.containsAttribute(Attribute.ICE_CONTROLLING))
- {
- IceControllingAttribute controlling = (IceControllingAttribute)
- req.getAttribute(Attribute.ICE_CONTROLLING);
-
- long theirTieBreaker = controlling.getTieBreaker();
+ final boolean bothControllingConflict = selfIceControlling &&
+ req.containsAttribute(Attribute.ICE_CONTROLLING);
- // If the agent's tie-breaker is larger than or equal to the
- // contents of the ICE-CONTROLLING attribute, the agent generates
- // a Binding error response and includes an ERROR-CODE attribute
- // with a value of 487 (Role Conflict) but retains its role.
- if(compareUnsignedLong(ourTieBreaker, theirTieBreaker) >= 0)
- {
- Response response = MessageFactory.createBindingErrorResponse(
- ErrorCodeAttribute.ROLE_CONFLICT);
-
- try
- {
- stunStack.sendResponse(
- evt.getTransactionID().getBytes(),
- response,
- evt.getLocalAddress(),
- evt.getRemoteAddress());
-
- return false;
- }
- catch(Exception exc)
- {
- //rethrow so that we would send a 500 response instead.
- throw new RuntimeException("Failed to send a 487", exc);
- }
- }
- //If the agent's tie-breaker is less than the contents of the
- //ICE-CONTROLLING attribute, the agent switches to the controlled
- //role.
- else
- {
- logger.trace(() ->
- "Switching to controlled because theirTieBreaker="
- + theirTieBreaker + " and ourTieBreaker="
- + ourTieBreaker);
- parentAgent.setControlling(false);
- return true;
- }
- }
// If the agent is in the controlled role, and the ICE-CONTROLLED
// attribute is present in the request:
- else if(!parentAgent.isControlling()
- && req.containsAttribute(Attribute.ICE_CONTROLLED))
+ final boolean bothControlledConflict = !selfIceControlling &&
+ req.containsAttribute(Attribute.ICE_CONTROLLED);
+
+ if (!(bothControllingConflict || bothControlledConflict)) {
+ // we don't have a role conflict
+ return true;
+ }
+
+ final long selfTieBreaker = parentAgent.getTieBreaker();
+
+ final IceControlAttribute theirIceControl = bothControllingConflict
+ ? (IceControlAttribute)req.getAttribute(Attribute.ICE_CONTROLLING)
+ : (IceControlAttribute)req.getAttribute(Attribute.ICE_CONTROLLED);
+
+ final long theirTieBreaker = theirIceControl.getTieBreaker();
+
+ // If the agent's tie-breaker is larger than or equal to the
+ // contents of the ICE control attribute, the agent generates
+ // a Binding error response and includes an ERROR-CODE attribute
+ // with a value of 487 (Role Conflict) but retains its role.
+ if (Long.compareUnsigned(selfTieBreaker, theirTieBreaker) >= 0)
{
- IceControlledAttribute controlled = (IceControlledAttribute)
- req.getAttribute(Attribute.ICE_CONTROLLED);
+ final UsernameAttribute requestUserName = (UsernameAttribute)req
+ .getAttribute(Attribute.USERNAME);
+
+ final Response response =
+ MessageFactory.createBindingErrorResponse(
+ ErrorCodeAttribute.ROLE_CONFLICT);
- long theirTieBreaker = controlled.getTieBreaker();
+ final Attribute messageIntegrityAttribute =
+ AttributeFactory.createMessageIntegrityAttribute(
+ new String(requestUserName.getUsername()));
+ response.putAttribute(messageIntegrityAttribute);
- //If the agent's tie-breaker is larger than or equal to the
- //contents of the ICE-CONTROLLED attribute, the agent switches to
- //the controlling role.
- if(compareUnsignedLong(ourTieBreaker, theirTieBreaker) >= 0)
+ try
{
- logger.trace(() ->
- "Switching to controlling because theirTieBreaker="
- + theirTieBreaker + " and ourTieBreaker="
- + ourTieBreaker);
- parentAgent.setControlling(true);
- return true;
+ stunStack.sendResponse(
+ evt.getTransactionID().getBytes(),
+ response,
+ evt.getLocalAddress(),
+ evt.getRemoteAddress());
+ return false;
}
- // If the agent's tie-breaker is less than the contents of the
- // ICE-CONTROLLED attribute, the agent generates a Binding error
- // response and includes an ERROR-CODE attribute with a value of
- // 487 (Role Conflict) but retains its role.
- else
+ catch(Exception exc)
{
- Response response = MessageFactory.createBindingErrorResponse(
- ErrorCodeAttribute.ROLE_CONFLICT);
-
- try
- {
- stunStack.sendResponse(
- evt.getTransactionID().getBytes(),
- response,
- evt.getLocalAddress(),
- evt.getRemoteAddress());
-
- return false;
- }
- catch(Exception exc)
- {
- //rethrow so that we would send a 500 response instead.
- throw new RuntimeException("Failed to send a 487", exc);
- }
+ //rethrow so that we would send a 500 response instead.
+ throw new RuntimeException("Failed to send a 487", exc);
}
}
- return true; // we don't have a role conflict
+ //If the agent's tie-breaker is less than the contents of the
+ //ICE control attribute, the agent toggles its ICE control role.
+ else
+ {
+ final String selfNextControlState
+ = selfIceControlling ? "controlled" : "controlling";
+ logger.trace(() ->
+ "Switching to " + selfNextControlState + " because " +
+ " theirTieBreaker= " + theirTieBreaker + " and " +
+ "selfTieBreaker= " + selfTieBreaker);
+ parentAgent.setControlling(!selfIceControlling);
+ return true;
+ }
}
/**
diff --git a/src/test/java/org/ice4j/ice/RoleConflictResolutionTest.java b/src/test/java/org/ice4j/ice/RoleConflictResolutionTest.java
new file mode 100644
index 00000000..75303cee
--- /dev/null
+++ b/src/test/java/org/ice4j/ice/RoleConflictResolutionTest.java
@@ -0,0 +1,198 @@
+/*
+ * ice4j, the OpenSource Java Solution for NAT and Firewall Traversal.
+ *
+ * Copyright @ 2019 8x8, Inc
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.ice4j.ice;
+
+import org.ice4j.*;
+import org.junit.*;
+
+import java.io.*;
+import java.util.*;
+import java.util.concurrent.*;
+import java.util.logging.*;
+
+/**
+ * RoleConflictResolutionTest set of tests which does end-to-end test
+ * of ICE role conflict recovery.
+ *
+ * @author Yura Yaroshevich
+ */
+public class RoleConflictResolutionTest
+{
+ protected static final Logger logger
+ = Logger.getLogger(RoleConflictResolutionTest.class.getName());
+
+ @Test
+ public void testRecoveryFromBothControlledConflict()
+ throws Throwable
+ {
+ testRecoveryFromRoleConflict(false);
+ }
+
+ @Test
+ public void testRecoveryFromBothControllingConflict()
+ throws Throwable
+ {
+ testRecoveryFromRoleConflict(true);
+ }
+
+ private static void testRecoveryFromRoleConflict(boolean bothControlling)
+ throws Throwable
+ {
+ final Agent peer1 = createPeer("[peer-1]", bothControlling);
+ // Set explicit tie-breakers to avoid automatic conflict resolution
+ peer1.setTieBreaker(1);
+
+ final Agent peer2 = createPeer("[peer-2]", bothControlling);
+ peer1.setTieBreaker(2);
+
+ final CountDownLatch countDownLatch = new CountDownLatch(2);
+
+ for (Agent peer : Arrays.asList(peer1, peer2))
+ {
+ peer.addStateChangeListener(evt ->
+ {
+ logger.info(peer.toString() + ": state changed to "
+ + evt.toString());
+ if (peer.getState().isEstablished())
+ {
+ countDownLatch.countDown();
+ }
+ });
+ }
+
+ exchangeCredentials(peer1, peer2);
+ exchangeCandidates(peer1, peer2);
+
+ peer1.startConnectivityEstablishment();
+ peer2.startConnectivityEstablishment();
+
+ boolean isConnected = countDownLatch.await(20, TimeUnit.SECONDS);
+
+ logSelectedPairs(peer1);
+ logSelectedPairs(peer2);
+
+ Assert.assertTrue("Expected connection established within time out",
+ isConnected);
+ Assert.assertTrue("peer 1 connectivity",
+ peer1.getState().isEstablished());
+ Assert.assertTrue("peer 2 connectivity",
+ peer2.getState().isEstablished());
+
+ disposePeer(peer1);
+ disposePeer(peer2);
+ }
+
+ private static Agent createPeer(String label, boolean iceControlling)
+ throws IOException
+ {
+ final Agent agent = new Agent(label, null);
+ agent.setUseHostHarvester(true);
+ agent.setControlling(iceControlling);
+ IceMediaStream iceStream
+ = agent.createMediaStream("media-stream");
+
+ agent.createComponent(
+ iceStream,
+ Transport.UDP,
+ 0x400, 0x400, 0xFFFF,
+ KeepAliveStrategy.ALL_SUCCEEDED,
+ true);
+
+ return agent;
+ }
+
+ private static void disposePeer(Agent peer)
+ {
+ peer.free();
+ }
+
+ private static void logSelectedPairs(Agent peer)
+ {
+ for (IceMediaStream stream : peer.getStreams())
+ {
+ for (Component component : stream.getComponents())
+ {
+ CandidatePair selectedPair = component.getSelectedPair();
+ if (selectedPair != null)
+ {
+ logger.info( peer.toString() + ": selected pair for " +
+ "component " + component.getName() + " :" +
+ selectedPair.toString());
+ }
+ }
+ }
+ }
+
+ private static void exchangeCredentials(Agent peer1, Agent peer2)
+ {
+ for(IceMediaStream stream : peer2.getStreams())
+ {
+ stream.setRemoteUfrag(peer1.getLocalUfrag());
+ stream.setRemotePassword(peer1.getLocalPassword());
+ }
+
+ for(IceMediaStream stream : peer1.getStreams())
+ {
+ stream.setRemoteUfrag(peer2.getLocalUfrag());
+ stream.setRemotePassword(peer2.getLocalPassword());
+ }
+ }
+
+ private static void exchangeCandidates(Agent peer1, Agent peer2)
+ {
+ for (String streamName : peer1.getStreamNames())
+ {
+ IceMediaStream peer1Stream = peer1.getStream(streamName);
+ IceMediaStream peer2Stream = peer2.getStream(streamName);
+ if (peer1Stream == null || peer2Stream == null)
+ {
+ continue;
+ }
+
+ for (Integer id : peer1Stream.getComponentIDs())
+ {
+ Component peer1Component = peer1Stream.getComponent(id);
+ Component peer2Component = peer2Stream.getComponent(id);
+ if (peer1Component == null || peer2Component == null)
+ {
+ continue;
+ }
+
+ copyRemoteCandidates(peer1Component, peer2Component);
+ copyRemoteCandidates(peer2Component, peer1Component);
+ }
+ }
+ }
+
+ private static void copyRemoteCandidates(Component localComponent,
+ Component remoteComponent)
+ {
+ for(LocalCandidate candidate : remoteComponent.getLocalCandidates())
+ {
+ localComponent.addRemoteCandidate(
+ new RemoteCandidate(
+ candidate.getTransportAddress(),
+ localComponent,
+ candidate.getType(),
+ candidate.getFoundation(),
+ candidate.getPriority(),
+ null));
+ }
+ }
+}