ScionService logic (#18)

---------

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

CHANGELOG.md

@@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   [#14](https://github.com/tzaeschke/phtree-cpp/pull/14), 
   [#15](https://github.com/tzaeschke/phtree-cpp/pull/15),
   [#17](https://github.com/tzaeschke/phtree-cpp/pull/17)
+- BREAKING CHANGE:`ScionService` instances created via `Scion.newXYZ`
+  will not be considered by `Scion.defaultService()`. Also, `DatagramChannel.getService()`
+  does not create a service if none exists.
+  [#18](https://github.com/tzaeschke/phtree-cpp/pull/18)
 
 ### Fixed
 - Fixed: SCMP problem when pinging local AS.

doc/Design.md

@@ -15,6 +15,35 @@ Also, when roaming, the provider may not actually support SCION. In this case th
 device would need to connect to some kind of gateway to connect to either a daemon or
 at least a topology server + control server.
 
+## Server / Client
+
+Server functionality, such as `open()` and `receive()` -> `send(RequestPath)`, should _not_ require
+any access to a daemon or control service. The main reason is that it ensures efficiency
+by completely avoiding any type of interaction with daemon/CS.
+Interaction with daemon/CS should not be denied but it should not occur during default operations. 
+
+Client functionality will usually require a `ScionService`. If none is provided during `open()`,
+a service will internally be requested via `Scion.defaultService()`. 
+While explicit initialization is usually preferable as a design choice, this implicit
+initialization was implemented to allow usage of `open()` without additional arguments.
+This allows being much closer to the native Java `DatagramChannel` API.
+
+An alternative would be to push the the delayed/implicit initialization into the `ScionService`
+class. We would always have a `ScionService` instance, but it wouldn't initially connect to
+a daemon or control service. The disadvantage is that creation of a `ScionService` would have
+to fail "late", i.e. we can create it but only when we use it would be know whether it
+is actually able to connect to something.
+
+## ScionService
+
+The `ScionService` implements all interactions with a local daemon or with a control service.
+When a `ScionService` is required internally (e.g. by `DatagramChannel`) and none has been 
+specified, a `ScionService` will be requested (and if none has been created yet, created) via 
+`Scion.defaultService()`. 
+
+A `ScionService` created via specific factory methods (`Scion.newServiceWithDNS`, etc) will _not_ be
+used or returned by `Scion.defaultService()`.
+
 ## Library dependencies etc
 
 - Use Maven (instead of Gradle or something else): Maven is still the most used framework and arguable the best (

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

@@ -46,14 +46,20 @@ abstract class AbstractDatagramChannel<C extends AbstractDatagramChannel<?>> imp
   private Consumer<Scmp.Message> errorListener;
 
   protected AbstractDatagramChannel(ScionService service) throws IOException {
-    this.channel = java.nio.channels.DatagramChannel.open();
+    this(service, DatagramChannel.open());
+  }
+
+  protected AbstractDatagramChannel(
+      ScionService service, java.nio.channels.DatagramChannel channel) {
+    this.channel = channel;
     this.service = service;
   }
 
   protected synchronized void configureBlocking(boolean block) throws IOException {
     channel.configureBlocking(block);
   }
 
+  // `protected` because it should not be visible in ScmpChannel API.
   protected synchronized boolean isBlocking() {
     return channel.isBlocking();
   }
@@ -77,24 +83,15 @@ public synchronized void setPathPolicy(PathPolicy pathPolicy) throws IOException
     }
   }
 
-  public synchronized ScionService getService() {
-    // Late and implicit initialization is questionable but probably the best choice here.
-    // 1) Under any conceivable circumstances there should only be one service instance required,
-    //    so defaultService() should yield the 'correct' instance.
-    // 2) defaultService() should also be 'correct' in the sense that no other instance should ever
-    //    be required. The dedicated newServiceXYZ() methods should be unnecessary.
-    //
-    // Why do we do this? It allows us to use DatagramChannel.open() in all situations,
-    // independent of whether we are a client (requires Service) or a server (should not use
-    // Service).
+  protected synchronized ScionService getOrCreateService() {
     if (service == null) {
-      service = Scion.defaultService();
+      service = ScionService.defaultService();
     }
     return this.service;
   }
 
-  public synchronized void setService(ScionService service) {
-    this.service = service;
+  public synchronized ScionService getService() {
+    return this.service;
   }
 
   protected DatagramChannel channel() {
@@ -150,7 +147,7 @@ public synchronized C connect(SocketAddress addr) throws IOException {
       throw new IllegalArgumentException(
           "connect() requires an InetSocketAddress or a ScionSocketAddress.");
     }
-    return connect(pathPolicy.filter(getService().getPaths((InetSocketAddress) addr)));
+    return connect(pathPolicy.filter(getOrCreateService().getPaths((InetSocketAddress) addr)));
   }
 
   /**
@@ -185,7 +182,7 @@ public synchronized Path getCurrentPath() {
     return path;
   }
 
-  protected void setPath(RequestPath path) {
+  protected synchronized void setPath(RequestPath path) {
     this.path = path;
   }
 
@@ -362,7 +359,7 @@ protected void buildHeaderNoRefresh(
         channel.connect(connection);
       }
 
-      srcIA = getService().getLocalIsdAs();
+      srcIA = getOrCreateService().getLocalIsdAs();
       // Get external host address. This must be done *after* refreshing the path!
       InetSocketAddress srcSocketAddress = (InetSocketAddress) channel.getLocalAddress();
       srcAddress = srcSocketAddress.getAddress().getAddress();
@@ -392,7 +389,7 @@ protected RequestPath ensureUpToDate(RequestPath path) throws IOException {
 
   private RequestPath updatePath(RequestPath path) throws IOException {
     // expired, get new path
-    RequestPath newPath = pathPolicy.filter(getService().getPaths(path));
+    RequestPath newPath = pathPolicy.filter(getOrCreateService().getPaths(path));
 
     if (isConnected) { // equal to !isBound at this point
       if (!newPath.getFirstHopAddress().equals(this.connection)) {

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

@@ -29,22 +29,24 @@ public class DatagramChannel extends AbstractDatagramChannel<DatagramChannel>
   private final ByteBuffer bufferReceive;
   private final ByteBuffer bufferSend;
 
-  protected DatagramChannel() throws IOException {
-    this(null);
-  }
-
-  protected DatagramChannel(ScionService service) throws IOException {
-    super(service);
+  protected DatagramChannel(ScionService service, java.nio.channels.DatagramChannel channel)
+      throws IOException {
+    super(service, channel);
     this.bufferReceive = ByteBuffer.allocateDirect(getOption(StandardSocketOptions.SO_RCVBUF));
     this.bufferSend = ByteBuffer.allocateDirect(getOption(StandardSocketOptions.SO_SNDBUF));
   }
 
   public static DatagramChannel open() throws IOException {
-    return new DatagramChannel();
+    return open(null);
   }
 
   public static DatagramChannel open(ScionService service) throws IOException {
-    return new DatagramChannel(service);
+    return open(service, java.nio.channels.DatagramChannel.open());
+  }
+
+  public static DatagramChannel open(
+      ScionService service, java.nio.channels.DatagramChannel channel) throws IOException {
+    return new DatagramChannel(service, channel);
   }
 
   // TODO we return `void` here. If we implement SelectableChannel
@@ -85,7 +87,9 @@ public synchronized void send(ByteBuffer srcBuffer, SocketAddress destination)
     if (!(destination instanceof InetSocketAddress)) {
       throw new IllegalArgumentException("Address must be of type InetSocketAddress.");
     }
-    send(srcBuffer, getPathPolicy().filter(getService().getPaths((InetSocketAddress) destination)));
+    send(
+        srcBuffer,
+        getPathPolicy().filter(getOrCreateService().getPaths((InetSocketAddress) destination)));
   }
 
   /**

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

@@ -121,11 +121,6 @@ protected ScionService(String addressOrHost, Mode mode) {
       }
       throw e;
     }
-    synchronized (LOCK) {
-      if (DEFAULT == null) {
-        DEFAULT = this;
-      }
-    }
   }
 
   /**
@@ -171,10 +166,9 @@ static ScionService defaultService() {
         DEFAULT = new ScionService(daemonHost + ":" + daemonPort, Mode.DAEMON);
         return DEFAULT;
       } catch (ScionRuntimeException e) {
-        // Ignore
+        throw new ScionRuntimeException(
+            "Could not connect to daemon, DNS or bootstrap resource.", e);
       }
-
-      throw new ScionRuntimeException("Could not connect to daemon, DNS or bootstrap resource.");
     }
   }
 
@@ -229,6 +223,10 @@ public DatagramChannel openChannel() throws IOException {
     return DatagramChannel.open(this);
   }
 
+  public DatagramChannel openChannel(java.nio.channels.DatagramChannel channel) throws IOException {
+    return DatagramChannel.open(this, channel);
+  }
+
   Daemon.ASResponse getASInfo() {
     Daemon.ASRequest request = Daemon.ASRequest.newBuilder().setIsdAs(0).build();
     Daemon.ASResponse response;

src/test/java/org/scion/PackageVisibilityHelper.java

@@ -15,10 +15,12 @@
 package org.scion;
 
 import com.google.protobuf.ByteString;
+import com.google.protobuf.Timestamp;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.UnknownHostException;
 import java.nio.ByteBuffer;
+import java.time.Instant;
 import java.util.List;
 import org.scion.internal.InternalConstants;
 import org.scion.internal.ScionHeaderParser;
@@ -69,10 +71,24 @@ public static RequestPath createDummyPath(
         Daemon.Interface.newBuilder()
             .setAddress(Daemon.Underlay.newBuilder().setAddress(firstHopString).build())
             .build();
-    Daemon.Path path = Daemon.Path.newBuilder().setRaw(bs).setInterface(inter).build();
+    Timestamp ts = Timestamp.newBuilder().setSeconds(Instant.now().getEpochSecond() + 100).build();
+    Daemon.Path path =
+        Daemon.Path.newBuilder().setRaw(bs).setInterface(inter).setExpiration(ts).build();
     return RequestPath.create(path, dstIsdAs, dstHost, dstPort);
   }
 
+  public static ResponsePath createDummyResponsePath(
+      byte[] raw,
+      long srcIsdAs,
+      byte[] srcIP,
+      int srcPort,
+      long dstIsdAs,
+      byte[] dstIP,
+      int dstPort,
+      InetSocketAddress firstHop) {
+    return ResponsePath.create(raw, srcIsdAs, srcIP, srcPort, dstIsdAs, dstIP, dstPort, firstHop);
+  }
+
   public static RequestPath createRequestPath110_112(
       Daemon.Path.Builder builder,
       long dstIsdAs,

src/test/java/org/scion/api/DatagramChannelApiServerTest.java

@@ -0,0 +1,112 @@
+// Copyright 2023 ETH Zurich
+//
+// 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.scion.api;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.io.IOException;
+import java.net.*;
+import java.nio.ByteBuffer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.scion.*;
+import org.scion.testutil.ExamplePacket;
+import org.scion.testutil.MockDNS;
+import org.scion.testutil.MockDaemon;
+import org.scion.testutil.MockDatagramChannel;
+
+/**
+ * Test that typical "server" operations do not require a ScionService.
+ *
+ * <p>This is service-less operation is required for servers that should not use (or may not have)
+ * access to a daemon or control service. Service-less operation is mainly implemented to ensure
+ * good server performance by avoiding costly network calls to daemons etc.
+ */
+class DatagramChannelApiServerTest {
+
+  private static final int dummyPort = 44444;
+
+  @BeforeEach
+  public void beforeEach() throws IOException {
+    MockDaemon.closeDefault();
+    MockDNS.clear();
+    ScionService.closeDefault();
+  }
+
+  @AfterEach
+  public void afterEach() throws IOException {
+    MockDaemon.closeDefault();
+    MockDNS.clear();
+    ScionService.closeDefault();
+  }
+
+  @Test
+  void open_withoutService() throws IOException {
+    // check that open() (without service argument) does not internally require a ScionService.
+    try (DatagramChannel channel = DatagramChannel.open()) {
+      assertNull(channel.getService());
+    }
+  }
+
+  @Test
+  void bind_withoutService() throws IOException {
+    // check that open() (without service argument) does not internally require a ScionService.
+    try (DatagramChannel channel = DatagramChannel.open()) {
+      channel.bind(new InetSocketAddress("127.0.0.1", 12345));
+      assertNull(channel.getService());
+    }
+  }
+
+  @Test
+  void send_withoutService() throws IOException {
+    // check that send(ResponsePath) does not internally require a ScionService.
+    try (DatagramChannel channel = DatagramChannel.open()) {
+      assertNull(channel.getService());
+      ResponsePath path =
+          PackageVisibilityHelper.createDummyResponsePath(
+              new byte[0],
+              1,
+              new byte[4],
+              1,
+              1,
+              new byte[4],
+              1,
+              new InetSocketAddress("127.0.0.1", 1));
+      Path p2 = channel.send(ByteBuffer.allocate(0), path);
+      assertNull(channel.getService());
+      assertEquals(path, p2);
+    }
+  }
+
+  @Test
+  void receive_withoutService() throws IOException {
+    // check that receive() does not internally require a ScionService.
+    SocketAddress addr = new InetSocketAddress("127.0.0.1", 12345);
+    MockDatagramChannel mock = MockDatagramChannel.open();
+    mock.setReceiveCallback(
+        buf -> {
+          buf.put(ExamplePacket.PACKET_BYTES_SERVER_E2E_PING);
+          return addr;
+        });
+    try (DatagramChannel channel = DatagramChannel.open(null, mock)) {
+      assertNull(channel.getService());
+      ByteBuffer buffer = ByteBuffer.allocate(100);
+      Path path = channel.receive(buffer);
+      assertNull(channel.getService());
+      assertEquals(addr, path.getFirstHopAddress());
+    }
+  }
+}