Segment cleanup (#20)

---------

Co-authored-by: Tilmann Zäschke <tilmann.zaeschke@inf.ethz.ch>

CHANGELOG.md

@@ -7,7 +7,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 ### Added
 - Code coverage. [#11](https://github.com/tzaeschke/phtree-cpp/pull/11)
-- SCMP error handling: change paths in case of broken links or routers. 
 
 ### Changed
 - BREAKING CHANGE: Changed maven artifactId to "client"

TODO.md

@@ -15,7 +15,7 @@
   - Especially for type ¨5: External Interface Down" and "6: Internal Connectivity Down"
     Problem: we need to receive() or read() to actually receive SCMP errors.
     We could do this concurrently (actually, this would probably block writes),
-    or we do this onyl if the user calls read/receive. We can then store the failure info
+    or we do this only if the user calls read/receive. We can then store the failure info
     (path + AS/IP/IF of failure location). During next send/write, we compare the 
     path against this failure and try to find a better one. If no better path is found
     we can just drop the packet (default; consistent with UDP behavior) or throw an error. 

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

@@ -301,18 +301,20 @@ public synchronized <T> C setOption(SocketOption<T> option, T t) throws IOExcept
       } else {
         throw new UnsupportedOperationException();
       }
-    } else if (StandardSocketOptions.SO_RCVBUF.equals(option)) {
-      // TODO resize buf
-      channel.setOption(option, t);
-    } else if (StandardSocketOptions.SO_SNDBUF.equals(option)) {
-      // TODO resize buf
+    } else if (StandardSocketOptions.SO_RCVBUF.equals(option)
+        || StandardSocketOptions.SO_SNDBUF.equals(option)) {
       channel.setOption(option, t);
+      resizeBuffers(
+          channel.getOption(StandardSocketOptions.SO_RCVBUF),
+          channel.getOption(StandardSocketOptions.SO_SNDBUF));
     } else {
       channel.setOption(option, t);
     }
     return (C) this;
   }
 
+  protected abstract void resizeBuffers(int sizeReceive, int sizeSend);
+
   /**
    * @param path path
    * @param payloadLength payload length

src/main/java/org/scion/DatagramChannel.java

@@ -26,8 +26,8 @@
 public class DatagramChannel extends AbstractDatagramChannel<DatagramChannel>
     implements ByteChannel, Closeable {
 
-  private final ByteBuffer bufferReceive;
-  private final ByteBuffer bufferSend;
+  private ByteBuffer bufferReceive;
+  private ByteBuffer bufferSend;
 
   protected DatagramChannel(ScionService service, java.nio.channels.DatagramChannel channel)
       throws IOException {
@@ -61,6 +61,16 @@ public synchronized boolean isBlocking() {
     return super.isBlocking();
   }
 
+  @Override
+  protected void resizeBuffers(int sizeReceive, int sizeSend) {
+    if (bufferReceive.capacity() != sizeReceive) {
+      bufferReceive = ByteBuffer.allocateDirect(sizeReceive);
+    }
+    if (bufferSend.capacity() != sizeSend) {
+      bufferSend = ByteBuffer.allocateDirect(sizeSend);
+    }
+  }
+
   public synchronized ResponsePath receive(ByteBuffer userBuffer) throws IOException {
     ResponsePath receivePath = receiveFromChannel(bufferReceive, InternalConstants.HdrTypes.UDP);
     if (receivePath == null) {

src/main/java/org/scion/ScionService.java

@@ -63,11 +63,10 @@ public class ScionService {
 
   private static final String DNS_TXT_KEY = "scion";
   private static final Object LOCK = new Object();
-  private static ScionService DEFAULT = null;
   private static final String ERR_INVALID_TXT = "Invalid TXT entry: ";
+  private static ScionService defaultService = null;
 
   private final ScionBootstrapper bootstrapper;
-  // TODO create subclasses for these two? We can only have either one of them, not both.
   private final DaemonServiceGrpc.DaemonServiceBlockingStub daemonStub;
   private final SegmentLookupServiceGrpc.SegmentLookupServiceBlockingStub segmentStub;
 
@@ -133,28 +132,28 @@ static ScionService defaultService() {
     synchronized (LOCK) {
       // This is not 100% thread safe, but the worst that can happen is that
       // we call close() on a Service that has already been closed.
-      if (DEFAULT != null) {
-        return DEFAULT;
+      if (defaultService != null) {
+        return defaultService;
       }
       // try bootstrap service IP
       String fileName =
           ScionUtil.getPropertyOrEnv(PROPERTY_BOOTSTRAP_TOPO_FILE, ENV_BOOTSTRAP_TOPO_FILE);
       if (fileName != null) {
-        DEFAULT = new ScionService(fileName, Mode.BOOTSTRAP_TOPO_FILE);
-        return DEFAULT;
+        defaultService = new ScionService(fileName, Mode.BOOTSTRAP_TOPO_FILE);
+        return defaultService;
       }
 
       String server = ScionUtil.getPropertyOrEnv(PROPERTY_BOOTSTRAP_HOST, ENV_BOOTSTRAP_HOST);
       if (server != null) {
-        DEFAULT = new ScionService(server, Mode.BOOTSTRAP_SERVER_IP);
-        return DEFAULT;
+        defaultService = new ScionService(server, Mode.BOOTSTRAP_SERVER_IP);
+        return defaultService;
       }
 
       String naptrName =
           ScionUtil.getPropertyOrEnv(PROPERTY_BOOTSTRAP_NAPTR_NAME, ENV_BOOTSTRAP_NAPTR_NAME);
       if (naptrName != null) {
-        DEFAULT = new ScionService(naptrName, Mode.BOOTSTRAP_VIA_DNS);
-        return DEFAULT;
+        defaultService = new ScionService(naptrName, Mode.BOOTSTRAP_VIA_DNS);
+        return defaultService;
       }
 
       // try daemon
@@ -163,8 +162,8 @@ static ScionService defaultService() {
       String daemonPort =
           ScionUtil.getPropertyOrEnv(PROPERTY_DAEMON_PORT, ENV_DAEMON_PORT, DEFAULT_DAEMON_PORT);
       try {
-        DEFAULT = new ScionService(daemonHost + ":" + daemonPort, Mode.DAEMON);
-        return DEFAULT;
+        defaultService = new ScionService(daemonHost + ":" + daemonPort, Mode.DAEMON);
+        return defaultService;
       } catch (ScionRuntimeException e) {
         throw new ScionRuntimeException(
             "Could not connect to daemon, DNS or bootstrap resource.", e);
@@ -174,13 +173,13 @@ static ScionService defaultService() {
 
   public static void closeDefault() {
     synchronized (LOCK) {
-      if (DEFAULT != null) {
+      if (defaultService != null) {
         try {
-          DEFAULT.close();
+          defaultService.close();
         } catch (IOException e) {
           throw new ScionRuntimeException(e);
         } finally {
-          DEFAULT = null;
+          defaultService = null;
         }
       }
     }
@@ -191,9 +190,9 @@ private Thread addShutdownHook() {
         new Thread(
             () -> {
               try {
-                if (DEFAULT != null) {
-                  DEFAULT.shutdownHook = null;
-                  DEFAULT.close();
+                if (defaultService != null) {
+                  defaultService.shutdownHook = null;
+                  defaultService.close();
                 }
               } catch (IOException e) {
                 // Ignore, we just want to get out.

src/main/java/org/scion/ScionSocketOptions.java

@@ -17,7 +17,6 @@
 import java.net.SocketOption;
 
 public final class ScionSocketOptions {
-  // TODO options for Daemon ports/IPs?
 
   /**
    * If set to 'true', the Scion header parser will throw errors when encountering problems while
@@ -28,29 +27,17 @@ public final class ScionSocketOptions {
       new SciSocketOption<>("SN_API_THROW_PARSER_FAILURE", Boolean.class);
 
   /**
-   * TODO not yet implemented. If set to 'true', the receive() and read() operations will read new
-   * packets directly into the ByteBuffer provided by the user. The ByteBuffer will contain the
-   * header and the position of will be set to the first byte of the payload. This has two
-   * advantages: the payload does not need to be copied (saving one copy operation) and the Scion
-   * packet header is directly available to the user. If set to 'false', the receive() and read()
-   * operations will copy the payload to the ByteBuffer provided by the user. Default is 'false'
+   * If set to 'true', the receive() and read() operations will read new packets directly into the
+   * ByteBuffer provided by the user. The ByteBuffer will contain the header and the position of
+   * will be set to the first byte of the payload. This has two advantages: the payload does not
+   * need to be copied (saving one copy operation) and the Scion packet header is directly available
+   * to the user. If set to 'false', the receive() and read() operations will copy the payload to
+   * the ByteBuffer provided by the user. Default is 'false'
    */
   @Deprecated // TODO implement
   public static final SocketOption<Boolean> SN_API_WRITE_TO_USER_BUFFER =
       new SciSocketOption<>("SN_API_WRITE_TO_USER_BUFFER", Boolean.class);
 
-  // TODO can we use this size for the ByteBuffer in the channel? In the original socket
-  //   they probably refer to a different type of byffer, i.e. if packets are generated
-  //   faster than they can be sent.
-  //   -> The Channel only has *one* buffer....
-  //  /**  	The size of the socket send buffer. Default is 2000. */
-  //  public static final SocketOption<Integer> SSO_SNDBUF = new SciSocketOption<>("SSO_SNDBUF",
-  // Integer.class);;
-  //
-  //  /**  	The size of the socket receive buffer. Default is 2000. */
-  //  public static final SocketOption<Integer> SSO_RCVBUF = new SciSocketOption<>("SSO_RCVBUF",
-  // Integer.class);;
-
   /**
    * Before sending a packet, a new path will be requested if now() + pathExpirationMargin >
    * pathExpirationDate.

src/main/java/org/scion/ScmpChannel.java

@@ -127,8 +127,8 @@ private interface IOCallable<V> {
   }
 
   private class InternalChannel extends AbstractDatagramChannel<InternalChannel> {
-    private final ByteBuffer bufferReceive;
-    private final ByteBuffer bufferSend;
+    private ByteBuffer bufferReceive;
+    private ByteBuffer bufferSend;
     private final Selector selector;
 
     protected InternalChannel(ScionService service, RequestPath path, int port) throws IOException {
@@ -227,6 +227,16 @@ private ResponsePath receiveWithTimeout(ByteBuffer buffer) throws IOException {
       }
     }
 
+    @Override
+    protected void resizeBuffers(int sizeReceive, int sizeSend) {
+      if (bufferReceive.capacity() != sizeReceive) {
+        bufferReceive = ByteBuffer.allocateDirect(sizeReceive);
+      }
+      if (bufferSend.capacity() != sizeSend) {
+        bufferSend = ByteBuffer.allocateDirect(sizeSend);
+      }
+    }
+
     @Override
     public void close() throws IOException {
       super.close();

src/main/java/org/scion/internal/ByteUtil.java

@@ -16,13 +16,21 @@
 
 public class ByteUtil {
 
-  /** Mutable integer. */
-  public static class MutInt {
-    public int v;
+  /** Mutable long integer. */
+  public static class MutLong {
+    public long v;
 
-    MutInt(int v) {
+    MutLong(long v) {
       this.v = v;
     }
+
+    public long get() {
+      return v;
+    }
+
+    public void set(long l) {
+      this.v = l;
+    }
   }
 
   /**

src/main/java/org/scion/internal/ScionBootstrapper.java

@@ -103,7 +103,7 @@ private static String bootstrapViaDNS(String hostName) {
         if (STR_X_SCION_TCP.equals(naptrService)) {
           String host = nr.getReplacement().toString();
           String naptrFlag = nr.getFlags();
-          LOG.info("Found DNS entry: " + naptrService);
+          LOG.info("Found DNS entry: {}", naptrService);
           int port = queryTXT(hostName);
           if ("A".equals(naptrFlag)) {
             InetAddress addr = DNSHelper.queryA(host);
@@ -240,14 +240,14 @@ public int getLocalMtu() {
   }
 
   public void refreshTopology() {
-    // TODO check timeout from dig netsec-w37w3w.inf.ethz.ch?
+    // TODO check timeout from NAPTR record
     // TODO verify local DNS?? How?
     // init();
     throw new UnsupportedOperationException();
   }
 
   private String getTopologyFile() throws IOException {
-    LOG.info("Getting topology from bootstrap server: " + topologyResource);
+    LOG.info("Getting topology from bootstrap server: {}", topologyResource);
     controlServices.clear();
     discoveryServices.clear();
     // TODO https????

src/main/java/org/scion/internal/ScionHeaderParser.java

@@ -58,7 +58,6 @@ public static void extractUserPayload(ByteBuffer data, ByteBuffer userBuffer) {
    * @param data The datagram to read from.
    * @return A new ScionSocketAddress including raw path.
    */
-  // TODO this is a bit weird to have the firstHopAddress here....
   public static ResponsePath extractResponsePath(
       ByteBuffer data, InetSocketAddress firstHopAddress) {
     int pos = data.position();
@@ -337,7 +336,7 @@ public static void write(
     i0 = writeInt(i0, 12, 20, 1); // FlowID = 1
     data.putInt(i0);
     i1 = writeInt(i1, 0, 8, hdrType.code); // NextHdr = 17 is for UDP OverlayHeader
-    int newHdrLen = (calcLen(pathHeaderLength, dl, sl) - 1) / 4 + 1;
+    int newHdrLen = (calcLen(pathHeaderLength, sl, dl) - 1) / 4 + 1;
     i1 = writeInt(i1, 8, 8, newHdrLen); // HdrLen = bytes/4
     i1 = writeInt(i1, 16, 16, userPacketLength); // PayloadLen (+ overlay!)
     data.putInt(i1);