Merge pull request #289 from Mikunj/p2p-ping

Updated pinging logic.

CONTRIBUTING.md

@@ -89,11 +89,11 @@ can public keys. To test the P2P functionality on the same machine, however, req
 that each client binds their message server to a different port.
 
 You can use the following command to start a client bound to a different port.
+
 ```
 yarn start-multi
 ```
 
-
 For more than 2 clients, you can setup additional storage profiles and switch
 between them using the `NODE_APP_INSTANCE` environment variable and specifying a
 new localServerPort in the config.
@@ -149,14 +149,15 @@ So you wanna make a pull request? Please observe the following guidelines.
   the translations in
   [Transifex](https://www.transifex.com/projects/p/signal-desktop).
 -->
+
 * First, make sure that your `yarn ready` run passes - it's very similar to what our
   Continuous Integration servers do to test the app.
 * Never use plain strings right in the source code - pull them from `messages.json`!
   You **only** need to modify the default locale
   [`_locales/en/messages.json`](_locales/en/messages.json).
-<!-- TODO:
-  Other locales are generated automatically based on that file and then periodically
-  uploaded to Transifex for translation. -->
+  <!-- TODO:
+    Other locales are generated automatically based on that file and then periodically
+    uploaded to Transifex for translation. -->
 * [Rebase](https://nathanleclaire.com/blog/2014/09/14/dont-be-scared-of-git-rebase/) your
   changes on the latest `development` branch, resolving any conflicts.
   This ensures that your changes will merge cleanly when you open your PR.

js/modules/loki_p2p_api.js

@@ -1,6 +1,7 @@
 /* global setTimeout, clearTimeout */
 
 const EventEmitter = require('events');
+const { isEmpty } = require('lodash');
 
 const offlinePingTime = 2 * 60 * 1000; // 2 minutes
 
@@ -18,68 +19,59 @@ class LokiP2pAPI extends EventEmitter {
     });
   }
 
-  updateContactP2pDetails(pubKey, address, port, isPing = false) {
+  updateContactP2pDetails(pubKey, address, port, isP2PMessage = false) {
     // Stagger the timers so the friends don't ping each other at the same time
     const timerDuration =
       pubKey < this.ourKey
         ? 60 * 1000 // 1 minute
         : 2 * 60 * 1000; // 2 minutes
 
-    if (!this.contactP2pDetails[pubKey]) {
-      // We didn't have this contact's details
-      this.contactP2pDetails[pubKey] = {
-        address,
-        port,
-        timerDuration,
-        pingTimer: null,
-        isOnline: false,
-      };
-      if (isPing) {
-        this.setContactOnline(pubKey);
-        return;
-      }
-      // Try ping
-      this.pingContact(pubKey);
-      return;
-    }
-
-    // We already had this contact's details
-    const baseDetails = { ...this.contactP2pDetails[pubKey] };
-
-    if (isPing) {
-      // Received a ping
-      // Update details in case they are new and mark online
-      this.contactP2pDetails[pubKey].address = address;
-      this.contactP2pDetails[pubKey].port = port;
+    // Get the current contact details
+    // This will be empty if we don't have them
+    const baseDetails = { ...(this.contactP2pDetails[pubKey] || {}) };
+
+    // Always set the new contact details
+    this.contactP2pDetails[pubKey] = {
+      address,
+      port,
+      timerDuration,
+      pingTimer: null,
+      isOnline: false,
+    };
+
+    const contactExists = !isEmpty(baseDetails);
+    const { isOnline } = baseDetails;
+    const detailsChanged =
+      baseDetails.address !== address || baseDetails.port !== port;
+
+    // If we had the contact details
+    // And we got a P2P message
+    // And the contact was online
+    // And the new details that we got matched the old
+    // Then we don't need to bother pinging
+    if (contactExists && isP2PMessage && isOnline && !detailsChanged) {
+      // We also need to set the current contact details to show online
+      //  because they get reset to `false` above
       this.setContactOnline(pubKey);
       return;
     }
 
-    // Received a storage broadcast message
-    if (
-      baseDetails.isOnline ||
-      baseDetails.address !== address ||
-      baseDetails.port !== port
-    ) {
-      // Had the contact marked as online and details we had were the same
-      this.pingContact(pubKey);
-      return;
-    }
-
-    // Had the contact marked as offline or got new details
-    this.contactP2pDetails[pubKey].address = address;
-    this.contactP2pDetails[pubKey].port = port;
-    this.setContactOffline(pubKey);
+    /*
+      Ping the contact.
+      This happens in the following scenarios:
+        1. We didn't have the contact, we need to ping them to let them know our details.
+        2. isP2PMessage = false, so we assume the contact doesn't have our details.
+        3. We had the contact marked as offline,
+            we need to make sure that we can reach their server.
+        4. The other contact details have changed,
+            we need to make sure that we can reach their new server.
+    */
     this.pingContact(pubKey);
   }
 
   getContactP2pDetails(pubKey) {
-    return this.contactP2pDetails[pubKey] || null;
-  }
-
-  isContactOnline(pubKey) {
-    const contactDetails = this.contactP2pDetails[pubKey];
-    return !!(contactDetails && contactDetails.isOnline);
+    if (!this.contactP2pDetails[pubKey]) return null;
+    return { ...this.contactP2pDetails[pubKey] };
   }
 
   setContactOffline(pubKey) {

js/modules/loki_rpc.js

@@ -106,7 +106,10 @@ const rpc = (address, port, method, params, options = {}) => {
     method: 'POST',
     ...options,
     body: JSON.stringify(body),
-    headers,
+    headers: {
+      'Content-Type': 'application/json',
+      ...headers,
+    },
   };
 
   return fetch(url, fetchOptions);

libloki/test/node/loki_p2p_api_test.js

@@ -6,87 +6,188 @@ describe('LokiP2pAPI', () => {
   const usedAddress = 'anAddress';
   const usedPort = 'aPort';
 
+  const usedDetails = {
+    address: usedAddress,
+    port: usedPort,
+    timerDuration: 100,
+    pingTimer: null,
+    isOnline: false,
+  };
+
   beforeEach(() => {
     this.lokiP2pAPI = new LokiP2pAPI();
   });
 
   afterEach(() => {
+    this.lokiP2pAPI.removeAllListeners();
     this.lokiP2pAPI.reset();
   });
 
-  it("Should not emit a pingContact event if that contact doesn't exits", () => {
-    this.lokiP2pAPI.on('pingContact', () => {
-      assert.fail();
+  describe('getContactP2pDetails', () => {
+    it('Should return null if no contact details exist', () => {
+      const details = this.lokiP2pAPI.getContactP2pDetails(usedKey);
+      assert.isNull(details);
     });
-    this.lokiP2pAPI.pingContact('not stored');
-  });
 
-  it('Should emit an online event if the contact is online', done => {
-    this.lokiP2pAPI.on('online', pubKey => {
-      assert.strictEqual(pubKey, usedKey);
-      done();
+    it('Should return the exact same object if contact details exist', () => {
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = usedDetails;
+      const details = this.lokiP2pAPI.getContactP2pDetails(usedKey);
+      assert.deepEqual(details, usedDetails);
     });
-    this.lokiP2pAPI.updateContactP2pDetails(
-      usedKey,
-      usedAddress,
-      usedPort,
-      true
-    );
-  }).timeout(1000);
-
-  it("Should send a pingContact event if the contact isn't online", done => {
-    this.lokiP2pAPI.on('pingContact', pubKey => {
-      assert.strictEqual(pubKey, usedKey);
-      done();
-    });
-    this.lokiP2pAPI.updateContactP2pDetails(
-      usedKey,
-      usedAddress,
-      usedPort,
-      false
-    );
-  }).timeout(1000);
-
-  it('Should store a contacts p2p details', () => {
-    this.lokiP2pAPI.updateContactP2pDetails(
-      usedKey,
-      usedAddress,
-      usedPort,
-      true
-    );
-    const p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
-    assert.strictEqual(usedAddress, p2pDetails.address);
-    assert.strictEqual(usedPort, p2pDetails.port);
   });
 
-  it('Should say if a contact is online', () => {
-    this.lokiP2pAPI.updateContactP2pDetails(
-      usedKey,
-      usedAddress,
-      usedPort,
-      false
-    );
-    assert.isFalse(this.lokiP2pAPI.isOnline(usedKey));
-    this.lokiP2pAPI.updateContactP2pDetails(
-      usedKey,
-      usedAddress,
-      usedPort,
-      true
-    );
-    assert.isTrue(this.lokiP2pAPI.isOnline(usedKey));
+  describe('pingContact', () => {
+    it("Should not emit a pingContact event if that contact doesn't exits", () => {
+      this.lokiP2pAPI.on('pingContact', () => {
+        assert.fail();
+      });
+      this.lokiP2pAPI.pingContact('not stored');
+    });
   });
 
-  it('Should set a contact as offline', () => {
-    this.lokiP2pAPI.updateContactP2pDetails(
-      usedKey,
-      usedAddress,
-      usedPort,
-      true
-    );
-    let p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
-    assert.isTrue(p2pDetails.isOnline);
-    p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
-    this.lokiP2pAPI.setContactOffline(usedKey);
-    assert.isFalse(p2pDetails.isOnline);
+  describe('updateContactP2pDetails', () => {
+    it("Shouldn't ping a contact if contact exists, p2p message was sent, contact was online and details didn't change", () => {
+      this.lokiP2pAPI.on('pingContact', () => {
+        assert.fail();
+      });
+
+      // contact exists
+      const details = { ...usedDetails };
+      // P2p message
+      const isP2P = true;
+      // Contact was online
+      details.isOnline = true;
+      // details were the same
+      const { address, port } = details;
+
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = details;
+      this.lokiP2pAPI.updateContactP2pDetails(usedKey, address, port, isP2P);
+
+      // They should also be marked as online
+      assert.isTrue(this.lokiP2pAPI.isOnline(usedKey));
+    });
+
+    it("Should ping a contact if we don't have details for it", done => {
+      this.lokiP2pAPI.on('pingContact', pubKey => {
+        assert.strictEqual(pubKey, usedKey);
+        assert.isFalse(this.lokiP2pAPI.isOnline(usedKey));
+        done();
+      });
+      this.lokiP2pAPI.updateContactP2pDetails(
+        usedKey,
+        usedAddress,
+        usedPort,
+        true
+      );
+    });
+
+    it("Should ping a contact if a P2P message wasn't received", done => {
+      // The precondition for this is that we had the contact stored
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = { ...usedDetails };
+
+      this.lokiP2pAPI.on('pingContact', pubKey => {
+        assert.strictEqual(pubKey, usedKey);
+        assert.isFalse(this.lokiP2pAPI.isOnline(usedKey));
+        done();
+      });
+      this.lokiP2pAPI.updateContactP2pDetails(
+        usedKey,
+        usedAddress,
+        usedPort,
+        false // We didn't get a p2p message
+      );
+    });
+
+    it('Should ping a contact if they were marked as offline', done => {
+      // The precondition for this is that we had the contact stored
+      // And that p2p message was true
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = { ...usedDetails };
+
+      this.lokiP2pAPI.on('pingContact', pubKey => {
+        assert.strictEqual(pubKey, usedKey);
+        assert.isFalse(this.lokiP2pAPI.isOnline(usedKey));
+        done();
+      });
+      this.lokiP2pAPI.updateContactP2pDetails(
+        usedKey,
+        usedAddress,
+        usedPort,
+        true // We got a p2p message
+      );
+    });
+
+    it('Should ping a contact if the address was different', done => {
+      // The precondition for this is that we had the contact stored
+      // And that p2p message was true
+      // And that the user was online
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = { ...usedDetails };
+      this.lokiP2pAPI.contactP2pDetails[usedKey].isOnline = true;
+
+      this.lokiP2pAPI.on('pingContact', pubKey => {
+        assert.strictEqual(pubKey, usedKey);
+        done();
+      });
+      this.lokiP2pAPI.updateContactP2pDetails(
+        usedKey,
+        'different address',
+        usedPort,
+        true // We got a p2p message
+      );
+    });
+
+    it('Should ping a contact if the port was different', done => {
+      // The precondition for this is that we had the contact stored
+      // And that p2p message was true
+      // And that the user was online
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = { ...usedDetails };
+      this.lokiP2pAPI.contactP2pDetails[usedKey].isOnline = true;
+
+      this.lokiP2pAPI.on('pingContact', pubKey => {
+        assert.strictEqual(pubKey, usedKey);
+        done();
+      });
+      this.lokiP2pAPI.updateContactP2pDetails(
+        usedKey,
+        usedAddress,
+        'different port',
+        true // We got a p2p message
+      );
+    });
+
+    it('Should emit an online event if the contact is online', done => {
+      this.lokiP2pAPI.on('online', pubKey => {
+        assert.strictEqual(pubKey, usedKey);
+        done();
+      });
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = { ...usedDetails };
+      this.lokiP2pAPI.setContactOnline(usedKey);
+    }).timeout(1000);
+
+    it('Should store a contacts p2p details', () => {
+      this.lokiP2pAPI.updateContactP2pDetails(
+        usedKey,
+        usedAddress,
+        usedPort,
+        true
+      );
+      const p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
+      assert.strictEqual(usedAddress, p2pDetails.address);
+      assert.strictEqual(usedPort, p2pDetails.port);
+    });
+
+    it('Should set a contact as offline and online', () => {
+      this.lokiP2pAPI.contactP2pDetails[usedKey] = { ...usedDetails };
+      let p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
+      assert.isNotNull(p2pDetails);
+      assert.isFalse(p2pDetails.isOnline);
+      this.lokiP2pAPI.setContactOnline(usedKey);
+
+      p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
+      assert.isTrue(p2pDetails.isOnline);
+      this.lokiP2pAPI.setContactOffline(usedKey);
+
+      p2pDetails = this.lokiP2pAPI.getContactP2pDetails(usedKey);
+      assert.isFalse(p2pDetails.isOnline);
+    });
   });
 });