Path subclasses InetSocketAddress - v3

CHANGELOG.md

@@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
-## Added
+### Added
 - SCMP echo responder [#78](https://github.com/scionproto-contrib/jpan/pull/78)
 - Maven Java executor [#80](https://github.com/scionproto-contrib/jpan/pull/80)
 - Dev environment setup hints doc [#82](https://github.com/scionproto-contrib/jpan/pull/82)
@@ -19,6 +19,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   - Rename `DatagramSocket` to `ScionDatagramSopcketl` and move it to main package
   - Rename `ScionSocketOptions`  starting with `SN_` to `SCION_`
 - SCMP API changes. [#71](https://github.com/scionproto-contrib/jpan/pull/71)
+- Make `Path` inherit `InetSocketAddress` [#69](https://github.com/scionproto-contrib/jpan/pull/69) 
+  - **BREAKING CHANGE**: Access to path details such as metadata has been moved to 
+    `Path.getDetails()`.
+  - **BREAKING CHANGE**: ScionDatagramChannel.send(buffer, path) returns 'int'.
+TODO
+- Move expiryMargin to ScionService?
+- remove ScionAddress?
+- Remove getPaths(long dstIsdAs, InetSocketAddress dstScionAddress) -< ISD + Scion address!!!
+- Remove use of getHostName() -> InetAddress!
 
 ### Fixed
 - Fixed locking and resizing of buffers. [#68](https://github.com/scionproto-contrib/jpan/pull/68)

doc/Design.md

@@ -64,15 +64,35 @@ used or returned by `Scion.defaultService()`.
 
 ## Paths
 
-There are two types of paths (both inherit `Path`:
+`Path` contains a destination IA:IP:port, a raw path and a first hop IP:port.
 
+There are two subclasses of paths (both inherit `Path`):
+
 - `RequestPath` are used to send initial request. They are retrieved from a path service and contain
   meta information.
-- `ResponsePath` are used to respond to a client request. They are extracted from SCION packets.
+- `ResponsePath` are used to respond to a client request. They are extracted from SCION packets. 
+  They also contains origin ISD/AS:IP:port.
 
-`Path` contains a destination IA:IP:port, a raw path and a first hop IP:port.
-`RequestPath` also contains path meta information.
-`ResponsePath` also contains origin IA:IP:port.
+### Design choices - Subclassing InetSocketAddress
+
+`Path` is a subclass of `InetSocketAddress`. This is useful for `DatagramChannel`'s `receive()` 
+(which returns an `InetScoketAddress`) and `send()` which requires an `InetSocketAddress` as
+argument.
+
+The catch here is that `Path` has `getAddress()` and `getPort()` which are now contracted to 
+return the *remote* IP/port. Ideally they would be called `getRemoteAddress()` and`getRemotePort()` 
+but that is not possible. 
+
+### Design choices - Mutable & separate "details"
+
+For concurrent usage, `Path`s should be thread safe. THis would ideally be achieved by making them
+immutable. 
+The problem is that paths expire or can otherwise be invalidated. We could make this explicit and
+force API users to either manage expiry/invalidation manually. 
+
+The alternative is to move the path of `Path` that is mutable to a new class `PathDetails`.
+`PathDetails` is itself immutable and is referenced atomically from `Path`, so thread safe
+behaviour is still easily achievable.
 
 ## DatagramSocket
 

src/main/java/org/scion/jpan/AbstractDatagramChannel.java

@@ -22,7 +22,6 @@
 import java.nio.channels.ClosedChannelException;
 import java.nio.channels.DatagramChannel;
 import java.nio.channels.NotYetConnectedException;
-import java.time.Instant;
 import java.util.Objects;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
@@ -179,21 +178,18 @@
   /**
    * Returns the remote address.
    *
+   * @return The remote address or 'null' if this channel is not connected.
    * @see DatagramChannel#getRemoteAddress()
-   * @return The remote address.
+   * @see #connect(SocketAddress)
+   * @see #connect(RequestPath)
    * @throws IOException If an I/O error occurs
    */
   public InetSocketAddress getRemoteAddress() throws IOException {
-    Path path = getConnectionPath();
-    if (path != null) {
-      return new InetSocketAddress(path.getRemoteAddress(), path.getRemotePort());
-    }
-    return null;
+    return (InetSocketAddress) connectionPath;
   }
 
   public void disconnect() throws IOException {
     synchronized (stateLock) {
-      channel.disconnect(); // TODO Why ? We shouldn´t do that...?
       connectionPath = null;
     }
   }
@@ -214,12 +210,15 @@
   }
 
   /**
-   * Connect to a destination host. Note: - A SCION channel will internally connect to the next
-   * border router (first hop) instead of the remote host.
+   * Connect to a destination host.
+   *
+   * <p>NB: A SCION channel will internally connect to the next border router (first hop) instead of
+   * the remote host.
    *
-   * <p>NB: This method does internally no call {@link java.nio.channels.DatagramChannel}.connect(),
-   * instead it calls bind(). That means this method does NOT perform any additional security checks
-   * associated with connect(), only those associated with bind().
+   * <p>NB: This method does internally not call {@link
+   * java.nio.channels.DatagramChannel}.connect(), instead it calls bind(). That means this method
+   * does NOT perform any additional security checks associated with connect(), only those
+   * associated with bind().
    *
    * @param addr Address of remote host.
    * @return This channel.
@@ -232,7 +231,10 @@
         throw new IllegalArgumentException(
             "connect() requires an InetSocketAddress or a ScionSocketAddress.");
       }
-      return connect(pathPolicy.filter(getOrCreateService().getPaths((InetSocketAddress) addr)));
+      if (addr instanceof RequestPath) {
+        return connect(((RequestPath) addr));
+      }
+      return connect(getOrCreateService().lookupAndGetPath((InetSocketAddress) addr, pathPolicy));
     }
   }
 
@@ -301,7 +303,7 @@
    *
    * @return the current Path or `null` if not path is connected.
    */
-  public Path getConnectionPath() {
+  public RequestPath getConnectionPath() {
     synchronized (stateLock) {
       return connectionPath;
     }
@@ -398,17 +400,12 @@
     this.overrideExternalAddress = address;
   }
 
-  protected int sendRaw(ByteBuffer buffer, InetSocketAddress address, Path path)
-      throws IOException {
-    if (cfgRemoteDispatcher && path != null && path.getRawPath().length == 0) {
-      return channel.send(
-          buffer, new InetSocketAddress(address.getAddress(), Constants.DISPATCHER_PORT));
+  protected int sendRaw(ByteBuffer buffer, Path path) throws IOException {
+    if (cfgRemoteDispatcher && path.getRawPath().length == 0) {
+      InetAddress remoteHostIP = path.getFirstHopAddress().getAddress();
+      return channel.send(buffer, new InetSocketAddress(remoteHostIP, Constants.DISPATCHER_PORT));
     }
-    return channel.send(buffer, address);
-  }
-
-  protected int sendRaw(ByteBuffer buffer, InetSocketAddress address) throws IOException {
-    return sendRaw(buffer, address, null);
+    return channel.send(buffer, path.getFirstHopAddress());
   }
 
   public Consumer<Scmp.Message> setScmpErrorListener(Consumer<Scmp.Message> listener) {
@@ -536,18 +533,21 @@
   /**
    * @param path path
    * @param payloadLength payload length
-   * @return argument path or a new path if the argument path was expired
    * @throws IOException in case of IOException.
    */
-  protected Path checkPathAndBuildHeader(
+  protected void checkPathAndBuildHeader(
       ByteBuffer buffer, Path path, int payloadLength, InternalConstants.HdrTypes hdrType)
       throws IOException {
     synchronized (stateLock) {
       if (path instanceof RequestPath) {
-        path = ensureUpToDate((RequestPath) path);
+        RequestPath requestPath = (RequestPath) path;
+        ScionService svc = getOrCreateService();
+        if (svc.refreshPath(requestPath, getPathPolicy(), cfgExpirationSafetyMargin)
+            && isConnected()) {
+          updateConnection(requestPath, true);
+        }
       }
       buildHeader(buffer, path, payloadLength, hdrType);
-      return path;
     }
   }
 
@@ -574,6 +574,7 @@
         srcIA = rPath.getLocalIsdAs();
         srcAddress = rPath.getLocalAddress();
         srcPort = rPath.getLocalPort();
+        // TODO do we need to verify that the srcAddress is equal to the used local address?
       } else {
         srcIA = getOrCreateService().getLocalIsdAs();
         if (overrideExternalAddress != null) {
@@ -623,20 +624,6 @@
     }
   }
 
-  protected RequestPath ensureUpToDate(RequestPath path) throws IOException {
-    synchronized (stateLock) {
-      if (Instant.now().getEpochSecond() + cfgExpirationSafetyMargin <= path.getExpiration()) {
-        return path;
-      }
-      // expired, get new path
-      RequestPath newPath = pathPolicy.filter(getOrCreateService().getPaths(path));
-      if (isConnected()) {
-        updateConnection(newPath, true);
-      }
-      return newPath;
-    }
-  }
-
   private void updateConnection(RequestPath newPath, boolean mustBeConnected) throws IOException {
     if (mustBeConnected && !isConnected()) {
       throw new IllegalStateException();

src/main/java/org/scion/jpan/Path.java

@@ -15,55 +15,24 @@
 package org.scion.jpan;
 
 import java.net.*;
-import java.util.Arrays;
 
 /**
  * A Path is an InetSocketAddress/ISD/AS of a destination host plus a path to that host.
  *
  * <p>This class is thread safe.
  */
-public abstract class Path {
-  private final byte[] pathRaw;
-  private final long dstIsdAs;
-  private final InetAddress dstAddress;
-  private final int dstPort;
+public interface Path {
 
-  protected Path(byte[] rawPath, long dstIsdAs, InetAddress dstIP, int dstPort) {
-    this.pathRaw = rawPath;
-    this.dstIsdAs = dstIsdAs;
-    this.dstAddress = dstIP;
-    this.dstPort = dstPort;
-  }
+  byte[] getRawPath();
 
-  public byte[] getRawPath() {
-    return pathRaw;
-  }
+  InetSocketAddress getFirstHopAddress() throws UnknownHostException;
 
-  public abstract InetSocketAddress getFirstHopAddress() throws UnknownHostException;
+  int getRemotePort();
 
-  public int getRemotePort() {
-    return dstPort;
-  }
+  InetAddress getRemoteAddress();
 
-  public InetAddress getRemoteAddress() {
-    return dstAddress;
-  }
-
-  public long getRemoteIsdAs() {
-    return dstIsdAs;
-  }
+  long getRemoteIsdAs();
 
   @Override
-  public String toString() {
-    return "Path{"
-        + "rmtIsdAs="
-        + ScionUtil.toStringIA(dstIsdAs)
-        + ", rmtAddress="
-        + dstAddress
-        + ", rmtPort="
-        + dstPort
-        + ", pathRaw="
-        + Arrays.toString(pathRaw)
-        + '}';
-  }
+  String toString();
 }