[INTERNAL][CORE] Prepares Stream access #653

srossbach · 2019-08-18T01:11:17Z

This patch introduces an new class / interface which is just used for
the ITransmitter and IReceiver. This is necessary to distinguish between
plan stream connections and internal stuff.

This patch introduces new classes and interface. This is necessary to distinguish between plain stream connections and internal stuff. In general this patch is a whole rewrite of the DTM. It now manages two connection pools. One for packet connections and one for stream connections. In addition this commits adds Javadoc, TODOs and FIXMEs.

srossbach · 2019-08-27T16:24:54Z

@tobous can you just please have a look at the Javadoc and basically just the naming of the methods in the interfaces ? I guess the rest of this "commit" is not reviewable, however the whole logic of the DTM is tested with a bunch of test cases.

tobous · 2019-08-27T23:47:46Z

@srossbach Yes, I can have a look at it. But I'm busy for the rest of the week, so I won't get to it (or at least finish it) before next Monday.
Either way, with this mainly dealing with the network stuff, @stefaus should (also) have a look at this as it might interfere/intersect with his work.

srossbach · 2019-08-28T08:16:57Z

He has not replied since 9 days, so I removed him as reviewer.

tobous

I got through everything but the DataTransfereManager (and its test). For that class, I am way out of my depth (and I have been reviewing for a while, so my concentration is fading). I will try the class again in the next couple of days.

tobous · 2019-08-27T23:52:27Z

core/src/saros/net/IConnectionManager.java

+ * <p>It offers support for two connection types.
+ *
+ * <ol>
+ *   <li>Establishing a connection using {@link #connect(String, Object)} to establish a connection


Using the phrase "establish(ing) a connection" twice sounds a bit weird. But that is more of a nit-pick than a real complaint.

tobous · 2019-08-28T00:04:16Z

core/src/saros/net/IStreamConnectionListener.java

+   * @return <code>true</code> to accept the connection or <code>false</code> to reject the
+   *     connection.
+   */
+  public boolean connectionEstablished(IStreamConnection connection);


The public modifier is not needed for interface methods. Also, rename to better represent the "accepting/rejecting" nature?

Suggested change

public boolean connectionEstablished(IStreamConnection connection);

boolean checkIncomingConnection(IStreamConnection connection);

The problem is from a TCP (depends on the stream service) point of the the connection IS established, it occupies a port.

Ok. Then how about something along the lines of checkNewConnection(...)?

tobous · 2019-09-02T15:48:13Z

core/src/saros/net/internal/DataTransferManager.java

      try {
-        listener.connectionClosed(connectionId, connection);
+        accepted |= listener.connectionEstablished(connection);


The or-assignment is not necessary as you break anyways when accepted is true, i.e. when you encounter true for the first time.
(Also, you could avoid the unnecessary assignments by checking first and only assigning if the listener returns true and then breaking. But it makes the logic harder to read, so I'm not sure whether it's really worth it.

Suggested change

accepted |= listener.connectionEstablished(connection);

accepted = listener.connectionEstablished(connection);

tobous · 2019-09-03T13:55:06Z

core/src/saros/net/IStreamConnectionListener.java

+ * <p>Furthermore if a listener accepts the request every outstanding listener will <b>not</b> get
+ * notified about this connection establishment.
+ */
+public interface IStreamConnectionListener {


I am not sure I understand what this class is needed for. Could you explain the use case to me? Was it added so that we don't have to blindly accept any incoming stream?

Furthermore, using the name "listener" is a bit confusing in combination with the implemented behavior. This "listener" is only used to accept or reject connections and doesn't really do anything with the passed connection. Furthermore, it only runs until a registered listener accepts the connection, meaning not all listeners have to be notified (as stated in the javadoc).

I am struggling with finding a good name, but I somewhat like the metaphor of a bouncer for incoming connections (which doesn't completely match as all "bouncers" have to "reject" the connection for it to actually be rejected).

Suggested change

public interface IStreamConnectionListener {

public interface IStreamConnectionBouncer {

This is quite simple and has to do with (D)DOS attacks. So imagine you open a socket (you can only accept 65535 connections). The first check after calling accept (see ServerSocket) is to do some mighty "magic" and close the connection just afterwards if the "magic" decided you shall not pass. Now it gets passes to the listener(s). If no listener is interested, should I keep the connection alive ? I guess not.

TBH the name is bad. It should be changed.

tobous · 2019-09-03T13:59:10Z

core/src/saros/net/IStreamConnection.java

+ * A stream connection consists of an input and out stream. It up to the caller to gracefully
+ * shutdown the connections.
+ */
+public interface IStreamConnection extends IConnection {


Why does this interface provide access to the streams but also direct access to some of their functionality (the timeout stuff)?

Because timeout stuff is session management, The connection only provides the "stream" but not the session you would put above it. To clearify things, we are talking about network sessions (see OSI layer), not about a Saros session.

tobous · 2019-09-03T15:20:47Z

core/src/saros/net/internal/ConnectionPool.java

+
+  private final Map<String, IConnection> pool = new HashMap<>();
+
+  ConnectionPool(final String name) {


As far as I can tell, you want a pool for every connection type. You could also consider making the pool generic (<T extends IConnection>) in order to better differentiate between the connection types and make the usage easier. It doesn't really make the class more complicated and would me you wouldn't have to cast the result from get requests.

This would make the name somewhat unnecessary, but you could still keep it around for a better toString().

tobous · 2019-09-03T15:37:55Z

stf/src/saros/stf/server/rmi/controlbot/manipulation/impl/NetworkManipulatorImpl.java

@@ -319,10 +320,10 @@ public void unblockOutgoingSessionPackets(JID jid) throws RemoteException {
      return;
    }

-    IByteStreamConnection connection =
-        getDataTransferManager().getConnection(ISarosSession.SESSION_CONNECTION_ID, jid);
+    IConnection connection =


The returned connection is a IPacketConnection. Why not hold it as such? (This would also make the cast in l.336 unnecessary.)
Also, why do you check the type in the while loop in l.326? Can this check ever fail? Instead, you can just do a null-check.

I have basically two review modes.

Production = all the good stuff
Test Frameworks = just get this **** done.

However yes it seems that I did a refactor during multiple patches however this was already coded in such a bad way😆 after the first changes that I do not recognize any warnings here that may araised after the next refactoring steps.

tobous · 2019-09-03T15:45:34Z

core/src/saros/net/internal/DataTransferManager.java

- * @author coezbek
- * @author jurke
- */
-@Component(module = "net")
 public class DataTransferManager implements IConnectionListener, IConnectionManager {


Add a new javadoc?

tobous · 2019-09-03T16:05:52Z

core/src/saros/net/internal/DataTransferManager.java

  }

-  private IByteStreamConnection connectInternal(String connectionID, JID peer) throws IOException {
+  private IConnection connectInternal(
+      final String id, final JID address, final boolean isPlainStream) throws IOException {


This solution using the Boolean to determine the return type is a bit ugly in my opinion. But using generics gets very ugly fast as far as I can tell, so this isn't an option either.

Would splitting this method into a stream and non-stream variant be a possibility without creating too much code duplication? The logic is kind of long/complicated, so I don't have a good overview.

tobous · 2019-09-03T16:14:23Z

core/test/junit/saros/net/internal/DataTransferManagerTest.java

+      for (ByteStream byteStream : establishedStreams)
+        try {
+          byteStream.close();
+        } catch (IOException e) { // ignore}


Suggested change

} catch (IOException e) { // ignore}

} catch (IOException e) { // ignore

srossbach · 2019-09-10T03:30:35Z

I currently have limited time. Will reply on friday, saturday, or sunday.

srossbach force-pushed the pr/prepare_streams branch from c66a516 to ecb5e10 Compare August 20, 2019 17:21

saros-project deleted a comment Aug 20, 2019

srossbach requested a review from stefaus August 20, 2019 17:26

srossbach removed the request for review from stefaus August 27, 2019 04:53

srossbach force-pushed the pr/prepare_streams branch from ecb5e10 to 76d49fc Compare August 27, 2019 04:54

saros-project deleted a comment Aug 27, 2019

srossbach requested a review from tobous August 27, 2019 16:23

tobous requested changes Sep 3, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INTERNAL][CORE] Prepares Stream access #653

[INTERNAL][CORE] Prepares Stream access #653

srossbach commented Aug 18, 2019

srossbach commented Aug 27, 2019

tobous commented Aug 27, 2019 •

edited

Loading

srossbach commented Aug 28, 2019

tobous left a comment

tobous Aug 27, 2019

tobous Aug 28, 2019

srossbach Sep 3, 2019

tobous Sep 3, 2019

tobous Sep 2, 2019

tobous Sep 3, 2019

srossbach Sep 3, 2019

tobous Sep 3, 2019

srossbach Sep 3, 2019

tobous Sep 3, 2019

tobous Sep 3, 2019

srossbach Sep 3, 2019

tobous Sep 3, 2019

tobous Sep 3, 2019

tobous Sep 3, 2019

srossbach commented Sep 10, 2019

	public boolean connectionEstablished(IStreamConnection connection);
	boolean checkIncomingConnection(IStreamConnection connection);

	accepted \|= listener.connectionEstablished(connection);
	accepted = listener.connectionEstablished(connection);

	public interface IStreamConnectionListener {
	public interface IStreamConnectionBouncer {


		private final Map<String, IConnection> pool = new HashMap<>();

		ConnectionPool(final String name) {

	} catch (IOException e) { // ignore}
	} catch (IOException e) { // ignore

[INTERNAL][CORE] Prepares Stream access #653

Are you sure you want to change the base?

[INTERNAL][CORE] Prepares Stream access #653

Conversation

srossbach commented Aug 18, 2019

srossbach commented Aug 27, 2019

tobous commented Aug 27, 2019 • edited Loading

srossbach commented Aug 28, 2019

tobous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srossbach commented Sep 10, 2019

tobous commented Aug 27, 2019 •

edited

Loading