Don't throw exception on empty path list (#160)

* Don't throw on empty path list --------- Co-authored-by: Tilmann Zäschke <tilmann.zaeschke@inf.ethz.ch>
scionproto-contrib · Feb 4, 2025 · 65a96f6 · 65a96f6
1 parent 052bdb8
commit 65a96f6
Show file tree

Hide file tree

Showing 12 changed files with 82 additions and 76 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,16 +10,21 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### TODO for 0.5.0
 
 TODO
-- Rethink Exception on empty path list. Remove?!!
+- Ordering: expiration, MTU
+- Filter: min MTU
+- Selectors: PingLatency, ReversePath, ...
+- Fix PplPolicy.fromJson()
+
+- rename "group" to "mapping"
+- add "ordering": bw, hopcount, latency, asc, desc
 - Add composite PathPolicy, composed of a chain of policies. Or, add chain() function?
 - Add normal policy to PolicyGroup with "destination"
-- Add prefereince/ordering attribute to PplGroup: MIN_HOPS, MIN_LATENCY, MAX_BANDWIDTH
+- Add preference/ordering attribute to PplGroup: MIN_HOPS, MIN_LATENCY, MAX_BANDWIDTH
   - For MIN_LATENCY: Change it to add 10000 for each UNKNOWN -> Two unknowns are worse than 1...
 - Why do they have the duplicate hopPredicate in almost every test?
   .addAclEntry(true, "0-0#0").addAclEntry(denyStr)
   -> Find out and either remove duplicate or remove buildNoValidate()
 - Add PPL JSON+YAML export. Fix JSON import of multiple policies
-- Add scion-apps-PAN "Preferred" Path Policy
 
 
 - FABRID is currently in SCIONlab. WHen ported to scionproto, JPAN should show policies in
@@ -68,7 +73,7 @@ For example: `Path.getFirstHopAddress()`, `DatagramChannel.setPathPolicy()`
 - Added keep-alive protocol for NAT. [#151](https://github.com/scionproto-contrib/jpan/pull/151)
 - Added implementation of STUN responder (currently not needed)
   [#154](https://github.com/scionproto-contrib/jpan/pull/154)
-- Path policies `PplPolicy` and `PplPilicyGroup` created from Path Policy Language
+- Path policies `PplPolicy` and `PplPolicyGroup` created from Path Policy Language
   [#158](https://github.com/scionproto-contrib/jpan/pull/158)
 
 ### Changed
@@ -77,7 +82,9 @@ For example: `Path.getFirstHopAddress()`, `DatagramChannel.setPathPolicy()`
 - Changed checkstyle rules. [#153](https://github.com/scionproto-contrib/jpan/pull/143)
 - **BREAKING CHANGE** `PathPolicy.filter(..)` to return a `List` of paths. 
   [#159](https://github.com/scionproto-contrib/jpan/pull/159)
-
+- Policy filters should not need to throw Exceptions when the path list is empty.
+  [#160](https://github.com/scionproto-contrib/jpan/pull/160)
+
 ### Fixed
 
 - SHIM should not crash when receiving unparseable packet (e.g. dstPort = -1).

diff --git a/src/main/java/org/scion/jpan/AbstractDatagramChannel.java b/src/main/java/org/scion/jpan/AbstractDatagramChannel.java
@@ -22,6 +22,7 @@
 import java.nio.channels.ClosedChannelException;
 import java.nio.channels.DatagramChannel;
 import java.nio.channels.NotYetConnectedException;
+import java.util.List;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
 import org.scion.jpan.internal.*;
@@ -96,13 +97,23 @@ public void setPathPolicy(PathPolicy pathPolicy) {
     synchronized (stateLock) {
       this.pathPolicy = pathPolicy;
       if (isConnected()) {
+        ScionSocketAddress destination = connectionPath.getRemoteSocketAddress();
         connectionPath =
-            (RequestPath) pathPolicy.filter(getService().getPaths(connectionPath)).get(0);
+            (RequestPath) applyFilter(getService().getPaths(connectionPath), destination).get(0);
         updateConnection(connectionPath, true);
       }
     }
   }
 
+  protected List<Path> applyFilter(List<Path> paths, Object address) throws ScionRuntimeException {
+    List<Path> filtered = getPathPolicy().filter(paths);
+    if (filtered.isEmpty()) {
+      String isdAs = ScionUtil.toStringIA(paths.get(0).getRemoteIsdAs());
+      throw new ScionRuntimeException("No path found to destination: " + isdAs + " --- " + address);
+    }
+    return filtered;
+  }
+
   /**
    * Return the ScionService used by this channel. A ScionService is, for example, required for
    * lookup up a path from a daemon or control server. See also {@link
@@ -203,7 +214,7 @@ public InetSocketAddress getLocalAddress() throws IOException {
   public InetSocketAddress getRemoteAddress() throws IOException {
     Path path = getConnectionPath();
     if (path != null) {
-      return new InetSocketAddress(path.getRemoteAddress(), path.getRemotePort());
+      return path.getRemoteSocketAddress();
     }
     return null;
   }
@@ -260,7 +271,8 @@ public C connect(SocketAddress addr) throws IOException {
       if (addr instanceof ScionSocketAddress) {
         return connect(((ScionSocketAddress) addr).getPath());
       }
-      Path path = pathPolicy.filter(getService().lookupPaths((InetSocketAddress) addr)).get(0);
+      InetSocketAddress destination = (InetSocketAddress) addr;
+      Path path = applyFilter(getService().lookupPaths(destination), destination).get(0);
       return connect(path);
     }
   }

diff --git a/src/main/java/org/scion/jpan/PathPolicy.java b/src/main/java/org/scion/jpan/PathPolicy.java
@@ -21,16 +21,13 @@
  * Path policy interface.
  *
  * <p>Contract:<br>
- * - The filter method must return a non-empty list of paths or throw a NoSuchElementException.<br>
- * - The list of paths returned by filter must be ordered by preference (most preferred first).<br>
+ * - The list of paths returned by filter may be ordered by preference (most preferred first).<br>
  * - The filter method must not modify the input list of paths (TBD).<br>
  * - The filter method must not keep a reference to the returned list. The returned list may be
  * modified by the caller.<br>
  * - The filter method must not return a list containing paths with null metadata.<br>
- * -
  */
 public interface PathPolicy {
-  String NO_PATH = "No path found to destination.";
   PathPolicy FIRST = new First();
   PathPolicy MAX_BANDWIDTH = new MaxBandwith();
   PathPolicy MIN_LATENCY = new MinLatency();
@@ -39,7 +36,7 @@ public interface PathPolicy {
 
   class First implements PathPolicy {
     public List<Path> filter(List<Path> paths) {
-      return assertNotEmpty(paths);
+      return paths;
     }
   }
 
@@ -52,32 +49,30 @@ public List<Path> filter(List<Path> paths) {
             int bw2 = Collections.min(p2.getMetadata().getBandwidthList()).intValue();
             return Integer.compare(bw2, bw1);
           });
-      return assertNotEmpty(result);
+      return result;
     }
   }
 
   class MinLatency implements PathPolicy {
     public List<Path> filter(List<Path> paths) {
       // A 0-value indicates that the AS did not announce a latency for this hop.
       // We use Integer.MAX_VALUE for comparison of these ASes.
-      return assertNotEmpty(
-          paths.stream()
-              .sorted(
-                  Comparator.comparing(
-                      path ->
-                          path.getMetadata().getLatencyList().stream()
-                              .mapToLong(l -> l >= 0 ? l : Integer.MAX_VALUE)
-                              .reduce(0, Long::sum)))
-              .collect(Collectors.toList()));
+      return paths.stream()
+          .sorted(
+              Comparator.comparing(
+                  path ->
+                      path.getMetadata().getLatencyList().stream()
+                          .mapToLong(l -> l >= 0 ? l : Integer.MAX_VALUE)
+                          .reduce(0, Long::sum)))
+          .collect(Collectors.toList());
     }
   }
 
   class MinHopCount implements PathPolicy {
     public List<Path> filter(List<Path> paths) {
-      return assertNotEmpty(
-          paths.stream()
-              .sorted(Comparator.comparing(path -> path.getMetadata().getInterfacesList().size()))
-              .collect(Collectors.toList()));
+      return paths.stream()
+          .sorted(Comparator.comparing(path -> path.getMetadata().getInterfacesList().size()))
+          .collect(Collectors.toList());
     }
   }
 
@@ -90,7 +85,7 @@ public IsdAllow(Set<Integer> allowedIsds) {
 
     @Override
     public List<Path> filter(List<Path> paths) {
-      return assertNotEmpty(paths.stream().filter(this::checkPath).collect(Collectors.toList()));
+      return paths.stream().filter(this::checkPath).collect(Collectors.toList());
     }
 
     private boolean checkPath(Path path) {
@@ -113,7 +108,7 @@ public IsdDisallow(Set<Integer> disallowedIsds) {
 
     @Override
     public List<Path> filter(List<Path> paths) {
-      return assertNotEmpty(paths.stream().filter(this::checkPath).collect(Collectors.toList()));
+      return paths.stream().filter(this::checkPath).collect(Collectors.toList());
     }
 
     private boolean checkPath(Path path) {
@@ -127,13 +122,6 @@ private boolean checkPath(Path path) {
     }
   }
 
-  static List<Path> assertNotEmpty(List<Path> paths) {
-    if (paths.isEmpty()) {
-      throw new NoSuchElementException(NO_PATH);
-    }
-    return paths;
-  }
-
   /**
    * @param paths A list of candidate paths
    * @return A list of path ordered by preference (most preferec first).

diff --git a/src/main/java/org/scion/jpan/ScionDatagramChannel.java b/src/main/java/org/scion/jpan/ScionDatagramChannel.java
@@ -136,7 +136,7 @@ public int send(ByteBuffer srcBuffer, SocketAddress destination) throws IOExcept
     synchronized (stateLock()) {
       path = resolvedDestinations.get(dst);
       if (path == null) {
-        path = (RequestPath) getPathPolicy().filter(getService().lookupPaths(dst)).get(0);
+        path = (RequestPath) applyFilter(getService().lookupPaths(dst), dst).get(0);
         resolvedDestinations.put(dst, path);
       }
     }
@@ -282,7 +282,8 @@ private RequestPath refreshPath(RequestPath path, RefreshPolicy refreshPolicy) {
         }
         throw new ScionRuntimeException("Path is expired");
       case POLICY:
-        return (RequestPath) getPathPolicy().filter(getService().getPaths(path)).get(0);
+        return (RequestPath)
+            applyFilter(getService().getPaths(path), path.getRemoteSocketAddress()).get(0);
       case SAME_LINKS:
         return findPathSameLinks(paths, path);
       default:

diff --git a/src/main/java/org/scion/jpan/ScionDatagramSocket.java b/src/main/java/org/scion/jpan/ScionDatagramSocket.java
@@ -310,15 +310,15 @@ public void send(DatagramPacket packet) throws IOException {
         synchronized (pathCache) {
           path = pathCache.get(addr);
           if (path == null) {
-            path = channel.getPathPolicy().filter(channel.getService().lookupPaths(addr)).get(0);
+            path = channel.applyFilter(channel.getService().lookupPaths(addr), addr).get(0);
           } else if (path instanceof RequestPath
               && path.getMetadata().getExpiration() > Instant.now().getEpochSecond()) {
             // check expiration only for RequestPaths
             RequestPath request = (RequestPath) path;
-            path = channel.getPathPolicy().filter(channel.getService().getPaths(request)).get(0);
+            path = channel.applyFilter(channel.getService().getPaths(request), addr).get(0);
           }
           if (path == null) {
-            throw new IOException("Address is not resolvable in SCION: " + packet.getAddress());
+            throw new IOException("Address is not resolvable in SCION: " + addr.getAddress());
           }
           pathCache.put(addr, path);
         }

diff --git a/src/main/java/org/scion/jpan/ppl/PplPolicy.java b/src/main/java/org/scion/jpan/ppl/PplPolicy.java
@@ -32,12 +32,6 @@
 // Copied from https://github.com/scionproto/scion/tree/master/private/path/pathpol
 public class PplPolicy implements PathPolicy {
 
-  // PolicyMap is a container for Policies, keyed by their unique name. PolicyMap
-  // can be used to marshal Policies to JSON. Unmarshaling back to PolicyMap is
-  // guaranteed to yield an object that is identical to the initial one.
-  // type PolicyMap map[string]*ExtPolicy
-  // TODO
-
   /** FilterOptions contains options for filtering. */
   static class FilterOptions {
     // IgnoreSequence can be used to ignore the sequence part of policies.
@@ -78,23 +72,15 @@ public static PplPolicy fromJson(String json) {
     return parseJsonFile(json).get(0); // TODO
   }
 
+  // Filter filters the paths according to the policy.
   @Override
   public List<Path> filter(List<Path> paths) {
-    List<Path> filtered = filterAll(paths);
-    return PathPolicy.assertNotEmpty(filtered);
-  }
-
-  // Filter filters the paths according to the policy.
-  List<Path> filterAll(List<Path> paths) {
     return filterOpt(paths, new FilterOptions(false));
   }
 
   // FilterOpt filters the path set according to the policy with the given
   // options.
   List<Path> filterOpt(List<Path> paths, FilterOptions opts) {
-    if (this == null) {
-      return paths;
-    }
     paths = acl == null ? paths : acl.eval(paths);
     if (sequence != null && !opts.ignoreSequence) {
       paths = sequence.eval(paths);

diff --git a/src/test/java/org/scion/jpan/api/DatagramChannelApiTest.java b/src/test/java/org/scion/jpan/api/DatagramChannelApiTest.java
@@ -24,6 +24,8 @@
 import java.nio.channels.NotYetConnectedException;
 import java.nio.charset.Charset;
 import java.time.Instant;
+import java.util.Collections;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.BiConsumer;
 import org.junit.jupiter.api.AfterAll;
@@ -385,6 +387,17 @@ void getPathPolicy() throws IOException {
     }
   }
 
+  @Test
+  void getPathPolicy_filterReturnsEmptyList() throws IOException {
+    try (ScionDatagramChannel channel = ScionDatagramChannel.open()) {
+      List<Path> paths = channel.getService().lookupPaths("127.0.0.1", 12345);
+      channel.connect(paths.get(0));
+      PathPolicy empty = paths1 -> Collections.emptyList();
+      Exception e = assertThrows(ScionRuntimeException.class, () -> channel.setPathPolicy(empty));
+      assertTrue(e.getMessage().startsWith("No path found to destination"));
+    }
+  }
+
   @Test
   void send_bufferSize() throws IOException {
     Path path = PackageVisibilityHelper.createMockRequestPath(null);

diff --git a/src/test/java/org/scion/jpan/api/PathPolicyLanguageGroupTest.java b/src/test/java/org/scion/jpan/api/PathPolicyLanguageGroupTest.java
@@ -1,4 +1,4 @@
-// Copyright 2024 ETH Zurich
+// Copyright 2025 ETH Zurich
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -21,7 +21,6 @@
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.NoSuchElementException;
 import java.util.Objects;
 import org.junit.jupiter.api.Test;
 import org.scion.jpan.*;
@@ -81,7 +80,7 @@ void filter_complex() {
     // policy_110a: address does not match -> policy_110b fails
     paths.clear();
     paths.add(createPath("10.0.0.3:12235", path133x110));
-    assertThrows(NoSuchElementException.class, () -> group.filter(paths));
+    assertTrue(group.filter(paths).isEmpty());
 
     // policy_110b - address match - no ISD match -> accept
     paths.clear();
@@ -93,7 +92,7 @@ void filter_complex() {
     paths.clear();
     addr = IPHelper.toInetSocketAddress("192.186.0.5:12234");
     paths.add(createPath(addr, "1-ff00:0:130", "0", "2", "1-ff00:0:110"));
-    assertThrows(NoSuchElementException.class, () -> group.filter(paths));
+    assertTrue(group.filter(paths).isEmpty());
 
     // default match only (ISD 1-..210) -> specific 112 -> accept
     paths.clear();
@@ -105,7 +104,7 @@ void filter_complex() {
     paths.clear();
     addr = IPHelper.toInetSocketAddress("192.186.0.5:12234");
     paths.add(createPath(addr, "1-ff00:0:113", "1", "2", "2-ff00:0:210"));
-    assertThrows(NoSuchElementException.class, () -> group.filter(paths));
+    assertTrue(group.filter(paths).isEmpty());
 
     // default match only  (ISD 1-..210) -> default ISD 2 -> accept
     paths.clear();
@@ -162,7 +161,7 @@ private void testDenyAllow(String destDeny, String addrDeny, String addrAllow) {
 
     // address+port do not match "deny"
     List<Path> pathsDeny = toList(createPath(addrAllow, path133x110));
-    assertThrows(NoSuchElementException.class, () -> group.filter(pathsDeny));
+    assertTrue(group.filter(pathsDeny).isEmpty());
   }
 
   /**

diff --git a/src/test/java/org/scion/jpan/api/PathPolicyLanguageTest.java b/src/test/java/org/scion/jpan/api/PathPolicyLanguageTest.java
@@ -1,4 +1,4 @@
-// Copyright 2024 ETH Zurich
+// Copyright 2025 ETH Zurich
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -18,7 +18,6 @@
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.NoSuchElementException;
 import org.junit.jupiter.api.Test;
 import org.scion.jpan.*;
 import org.scion.jpan.ppl.PplException;
@@ -51,7 +50,7 @@ void smokeTestBuilder() {
 
     List<Path> paths = new ArrayList<>();
     paths.add(ExamplePacket.PATH_IPV4);
-    assertThrows(NoSuchElementException.class, () -> ppl.filter(paths));
+    assertTrue(ppl.filter(paths).isEmpty());
 
     String str = PplPolicy.getSequence(ExamplePacket.PATH_IPV4); // TODO do we need this?
     assertEquals("", str);

diff --git a/src/test/java/org/scion/jpan/api/PathPolicyTest.java b/src/test/java/org/scion/jpan/api/PathPolicyTest.java
@@ -50,7 +50,7 @@ void minHops() {
   void minLatency() {
     List<Path> pathsWithDifferentLengths = createLongMixedList();
     List<Path> filtered = PathPolicy.MIN_LATENCY.filter(pathsWithDifferentLengths);
-    int prevLatency = 1;
+    int prevLatency = 0;
     for (int i = 0; i < pathsWithDifferentLengths.size(); i++) {
       int localMin = 0;
       for (Integer lat : filtered.get(i).getMetadata().getLatencyList()) {
@@ -65,7 +65,7 @@ void minLatency() {
         localMin = Integer.MAX_VALUE;
       }
 
-      assertTrue(localMin >= prevLatency);
+      assertTrue(localMin >= prevLatency, localMin + " vs " + prevLatency);
       prevLatency = localMin;
     }
   }