From b00a0cb699201bae36a52175b08d2465280fff99 Mon Sep 17 00:00:00 2001 From: Beaudan Date: Thu, 14 Feb 2019 14:49:24 +1100 Subject: [PATCH 1/7] Rename isOnline to isPing for clarity, and reduce the ttl for online broadcast messages to 1 min --- js/background.js | 4 ++-- js/modules/loki_message_api.js | 4 ++-- js/modules/loki_p2p_api.js | 4 ++-- libloki/api.js | 4 ++-- libtextsecure/outgoing_message.js | 8 ++++---- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/js/background.js b/js/background.js index 64fabbd0b..e5c6b6972 100644 --- a/js/background.js +++ b/js/background.js @@ -223,8 +223,8 @@ textsecure.storage.user.getNumber() ); window.lokiP2pAPI.on('pingContact', pubKey => { - const forceP2p = true; - window.libloki.api.sendOnlineBroadcastMessage(pubKey, forceP2p); + const isPing = true; + window.libloki.api.sendOnlineBroadcastMessage(pubKey, isPing); }); // These make key operations available to IPC handlers created in preload.js diff --git a/js/modules/loki_message_api.js b/js/modules/loki_message_api.js index 8aa1a387d..8034ff688 100644 --- a/js/modules/loki_message_api.js +++ b/js/modules/loki_message_api.js @@ -63,11 +63,11 @@ class LokiMessageAPI { this.messageServerPort = messageServerPort ? `:${messageServerPort}` : ''; } - async sendMessage(pubKey, data, messageTimeStamp, ttl, forceP2p = false) { + async sendMessage(pubKey, data, messageTimeStamp, ttl, isPing = false) { const data64 = dcodeIO.ByteBuffer.wrap(data).toString('base64'); const timestamp = Math.floor(Date.now() / 1000); const p2pDetails = lokiP2pAPI.getContactP2pDetails(pubKey); - if (p2pDetails && (forceP2p || p2pDetails.isOnline)) { + if (p2pDetails && (isPing || p2pDetails.isOnline)) { try { const port = p2pDetails.port ? `:${p2pDetails.port}` : ''; const url = `${p2pDetails.address}${port}/store`; diff --git a/js/modules/loki_p2p_api.js b/js/modules/loki_p2p_api.js index 349517c6b..aa7d8f726 100644 --- a/js/modules/loki_p2p_api.js +++ b/js/modules/loki_p2p_api.js @@ -16,7 +16,7 @@ class LokiP2pAPI extends EventEmitter { }); } - updateContactP2pDetails(pubKey, address, port, isOnline = false) { + updateContactP2pDetails(pubKey, address, port, isPing = false) { // Stagger the timers so the friends don't ping each other at the same time const timerDuration = pubKey < this.ourKey @@ -35,7 +35,7 @@ class LokiP2pAPI extends EventEmitter { pingTimer: null, }; - if (isOnline) { + if (isPing) { this.setContactOnline(pubKey); return; } diff --git a/libloki/api.js b/libloki/api.js index 2521fded3..e9c6dcc96 100644 --- a/libloki/api.js +++ b/libloki/api.js @@ -23,7 +23,7 @@ ); } - async function sendOnlineBroadcastMessage(pubKey, forceP2p = false) { + async function sendOnlineBroadcastMessage(pubKey, isPing = false) { const myLokiAddress = await window.lokiSnodeAPI.getMyLokiAddress(); const lokiAddressMessage = new textsecure.protobuf.LokiAddressMessage({ p2pAddress: `http://${myLokiAddress}`, @@ -41,7 +41,7 @@ log.info('Online broadcast message sent successfully'); } }; - const options = { messageType: 'onlineBroadcast', forceP2p }; + const options = { messageType: 'onlineBroadcast', isPing }; // Send a empty message with information about how to contact us directly const outgoingMessage = new textsecure.OutgoingMessage( null, // server diff --git a/libtextsecure/outgoing_message.js b/libtextsecure/outgoing_message.js index edf1c5fc9..4d940ae9c 100644 --- a/libtextsecure/outgoing_message.js +++ b/libtextsecure/outgoing_message.js @@ -42,13 +42,13 @@ function OutgoingMessage( this.failoverNumbers = []; this.unidentifiedDeliveries = []; - const { numberInfo, senderCertificate, online, messageType, forceP2p } = + const { numberInfo, senderCertificate, online, messageType, isPing } = options || {}; this.numberInfo = numberInfo; this.senderCertificate = senderCertificate; this.online = online; this.messageType = messageType || 'outgoing'; - this.forceP2p = forceP2p || false; + this.isPing = isPing || false; } OutgoingMessage.prototype = { @@ -192,7 +192,7 @@ OutgoingMessage.prototype = { data, timestamp, ttl, - this.forceP2p + this.isPing ); } catch (e) { if (e.name === 'HTTPError' && (e.code !== 409 && e.code !== 410)) { @@ -347,7 +347,7 @@ OutgoingMessage.prototype = { if (this.messageType === 'friend-request') { ttl = 4 * 24 * 60 * 60; // 4 days for friend request message } else if (this.messageType === 'onlineBroadcast') { - ttl = 10 * 60; // 10 minutes for online broadcast message + ttl = 60; // 1 minute for online broadcast message } else { const hours = window.getMessageTTL() || 24; // 1 day default for any other message ttl = hours * 60 * 60; From 25ded46e2cf18af339c4ef40582733d0759a6d27 Mon Sep 17 00:00:00 2001 From: Beaudan Date: Fri, 15 Feb 2019 14:56:17 +1100 Subject: [PATCH 2/7] Fixed some bugs removing nodes after the first failure instead of waiting for the failure count and also reduced the number of errors logged and made some warnings --- js/modules/loki_message_api.js | 18 ++++++++++++++---- js/modules/loki_snode_api.js | 23 ++++++----------------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/js/modules/loki_message_api.js b/js/modules/loki_message_api.js index 8034ff688..2906a9d8e 100644 --- a/js/modules/loki_message_api.js +++ b/js/modules/loki_message_api.js @@ -143,6 +143,7 @@ class LokiMessageAPI { nodeComplete(nodeUrl); successfulRequests += 1; } catch (e) { + log.warn('Send message error:', e); if (e instanceof NotFoundError) { canResolve = false; } else if (e instanceof HTTPError) { @@ -155,8 +156,12 @@ class LokiMessageAPI { // We mark the node as complete as we could still reach it nodeComplete(nodeUrl); } else { - log.error('Loki SendMessages:', e); - if (lokiSnodeAPI.unreachableNode(pubKey, nodeUrl)) { + const removeNode = await lokiSnodeAPI.unreachableNode( + pubKey, + nodeUrl + ); + if (removeNode) { + log.error('Loki SendMessages:', e); nodeComplete(nodeUrl); failedNodes.push(nodeUrl); } @@ -242,6 +247,7 @@ class LokiMessageAPI { } successfulRequests += 1; } catch (e) { + log.warn('Retrieve message error:', e); if (e instanceof NotFoundError) { canResolve = false; } else if (e instanceof HTTPError) { @@ -254,8 +260,12 @@ class LokiMessageAPI { // We mark the node as complete as we could still reach it nodeComplete(nodeUrl); } else { - log.error('Loki RetrieveMessages:', e); - if (lokiSnodeAPI.unreachableNode(ourKey, nodeUrl)) { + const removeNode = await lokiSnodeAPI.unreachableNode( + ourKey, + nodeUrl + ); + if (removeNode) { + log.error('Loki RetrieveMessages:', e); nodeComplete(nodeUrl); } } diff --git a/js/modules/loki_snode_api.js b/js/modules/loki_snode_api.js index 89563eacd..615b9367c 100644 --- a/js/modules/loki_snode_api.js +++ b/js/modules/loki_snode_api.js @@ -68,10 +68,11 @@ class LokiSnodeAPI { } else { this.ourSwarmNodes[nodeUrl].failureCount += 1; } - if (this.ourSwarmNodes[nodeUrl].failureCount >= FAILURE_THRESHOLD) { - delete this.ourSwarmNodes[nodeUrl]; + if (this.ourSwarmNodes[nodeUrl].failureCount < FAILURE_THRESHOLD) { + return false; } - return false; + delete this.ourSwarmNodes[nodeUrl]; + return true; } if (!this.contactSwarmNodes[nodeUrl]) { this.contactSwarmNodes[nodeUrl] = { @@ -85,7 +86,7 @@ class LokiSnodeAPI { } const conversation = ConversationController.get(pubKey); const swarmNodes = [...conversation.get('swarmNodes')]; - if (nodeUrl in swarmNodes) { + if (swarmNodes.includes(nodeUrl)) { const filteredNodes = swarmNodes.filter(node => node !== nodeUrl); await conversation.updateSwarmNodes(filteredNodes); delete this.contactSwarmNodes[nodeUrl]; @@ -161,7 +162,7 @@ class LokiSnodeAPI { newSwarmNodes = await this.getSwarmNodes(pubKey); } catch (e) { // TODO: Handle these errors sensibly - log.error('Failed to get new swarm nodes'); + log.error(e); newSwarmNodes = []; } resolve(newSwarmNodes); @@ -205,12 +206,6 @@ class LokiSnodeAPI { try { response = await fetch(options.url, fetchOptions); } catch (e) { - log.error( - options.type, - options.url, - 0, - `Error getting swarm nodes for ${pubKey}` - ); throw HTTPError('getSwarmNodes fetch error', 0, e.toString()); } @@ -229,12 +224,6 @@ class LokiSnodeAPI { if (response.status >= 0 && response.status < 400) { return result.nodes; } - log.error( - options.type, - options.url, - response.status, - `Error getting swarm nodes for ${pubKey}` - ); throw HTTPError('getSwarmNodes: error response', response.status, result); } } From d6a210efaa87840cbced3e793388dd7bd0e0d7e0 Mon Sep 17 00:00:00 2001 From: Beaudan Date: Fri, 15 Feb 2019 17:34:21 +1100 Subject: [PATCH 3/7] Reworked the update p2p details to be more robust and stopped some redundant pings from happening --- js/modules/loki_message_api.js | 4 +++ js/modules/loki_p2p_api.js | 48 ++++++++++++++++++++++++------- libtextsecure/message_receiver.js | 3 -- 3 files changed, 42 insertions(+), 13 deletions(-) diff --git a/js/modules/loki_message_api.js b/js/modules/loki_message_api.js index 2906a9d8e..a085a81c4 100644 --- a/js/modules/loki_message_api.js +++ b/js/modules/loki_message_api.js @@ -82,6 +82,10 @@ class LokiMessageAPI { } catch (e) { log.warn('Failed to send P2P message, falling back to storage', e); lokiP2pAPI.setContactOffline(pubKey); + if (isPing) { + // If this was just a ping, we don't bother sending to storage server + return; + } } } diff --git a/js/modules/loki_p2p_api.js b/js/modules/loki_p2p_api.js index aa7d8f726..fca8fbcf4 100644 --- a/js/modules/loki_p2p_api.js +++ b/js/modules/loki_p2p_api.js @@ -23,23 +23,47 @@ class LokiP2pAPI extends EventEmitter { ? 60 * 1000 // 1 minute : 2 * 60 * 1000; // 2 minutes - if (this.contactP2pDetails[pubKey]) { - clearTimeout(this.contactP2pDetails[pubKey].pingTimer); + if (!this.contactP2pDetails[pubKey]) { + // We didn't have this contact's details + this.contactP2pDetails[pubKey] = { + address, + port, + timerDuration, + pingTimer: null, + isOnline: false, + }; + // Try ping + this.pingContact(pubKey); + return; } - this.contactP2pDetails[pubKey] = { - address, - port, - timerDuration, - isOnline: false, - pingTimer: null, - }; + // 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; 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 + // Do nothing + return; + } + + // Had the contact marked as offline or got new details + this.contactP2pDetails[pubKey].address = address; + this.contactP2pDetails[pubKey].port = port; + this.setContactOffline(pubKey); this.pingContact(pubKey); } @@ -77,7 +101,11 @@ class LokiP2pAPI extends EventEmitter { } pingContact(pubKey) { - if (!this.contactP2pDetails[pubKey]) { + if ( + !this.contactP2pDetails[pubKey] || + this.contactP2pDetails[pubKey].isOnline + ) { + // Don't ping if we don't have their details or they are already online return; } this.emit('pingContact', pubKey); diff --git a/libtextsecure/message_receiver.js b/libtextsecure/message_receiver.js index 3a0204a81..934603148 100644 --- a/libtextsecure/message_receiver.js +++ b/libtextsecure/message_receiver.js @@ -218,9 +218,6 @@ MessageReceiver.prototype.extend({ const promise = Promise.resolve(request.body.toArrayBuffer()) // textsecure.crypto .then(plaintext => { const envelope = textsecure.protobuf.Envelope.decode(plaintext); - if (isP2p) { - lokiP2pAPI.setContactOnline(envelope.source); - } // After this point, decoding errors are not the server's // fault, and we should handle them gracefully and tell the // user they received an invalid message From 29bca71d5a179b6df02cacb0b21b99f69f6df40d Mon Sep 17 00:00:00 2001 From: Beaudan Date: Mon, 18 Feb 2019 12:46:19 +1100 Subject: [PATCH 4/7] Try ping contacts that send us storage server messages in the last 2 mins --- js/modules/loki_p2p_api.js | 17 +++++++++++------ libtextsecure/message_receiver.js | 10 ++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/js/modules/loki_p2p_api.js b/js/modules/loki_p2p_api.js index fca8fbcf4..941495b10 100644 --- a/js/modules/loki_p2p_api.js +++ b/js/modules/loki_p2p_api.js @@ -56,7 +56,7 @@ class LokiP2pAPI extends EventEmitter { baseDetails.port !== port ) { // Had the contact marked as online and details we had were the same - // Do nothing + this.pingContact(pubKey); return; } @@ -71,6 +71,14 @@ class LokiP2pAPI extends EventEmitter { return this.contactP2pDetails[pubKey] || null; } + isContactOnline(pubKey) { + const contactDetails = this.contactP2pDetails[pubKey]; + if (!contactDetails || !contactDetails.isOnline) { + return false; + } + return contactDetails.isOnline; + } + setContactOffline(pubKey) { this.emit('offline', pubKey); if (!this.contactP2pDetails[pubKey]) { @@ -101,11 +109,8 @@ class LokiP2pAPI extends EventEmitter { } pingContact(pubKey) { - if ( - !this.contactP2pDetails[pubKey] || - this.contactP2pDetails[pubKey].isOnline - ) { - // Don't ping if we don't have their details or they are already online + if (!this.contactP2pDetails[pubKey]) { + // Don't ping if we don't have their details return; } this.emit('pingContact', pubKey); diff --git a/libtextsecure/message_receiver.js b/libtextsecure/message_receiver.js index 934603148..985e42af3 100644 --- a/libtextsecure/message_receiver.js +++ b/libtextsecure/message_receiver.js @@ -945,6 +945,16 @@ MessageReceiver.prototype.extend({ return this.removeFromCache(envelope); }, handleDataMessage(envelope, msg) { + if (envelope.isP2p) { + lokiP2pAPI.setContactOnline(envelope.source); + } else { + const timestamp = envelope.timestamp.toNumber(); + const now = Date.now(); + const ageInSeconds = (now - timestamp) / 1000; + if (ageInSeconds <= 120) { + lokiP2pAPI.pingContact(envelope.source); + } + } window.log.info('data message from', this.getEnvelopeId(envelope)); let p = Promise.resolve(); // eslint-disable-next-line no-bitwise From dc67aaf9cc603429b3a3014a3e416d484ce9093d Mon Sep 17 00:00:00 2001 From: Beaudan Date: Mon, 18 Feb 2019 14:45:25 +1100 Subject: [PATCH 5/7] Roll back change to ping more often again, ping our offline contacts every 2 mins to check if they have come back online --- js/modules/loki_p2p_api.js | 7 +++++++ libtextsecure/message_receiver.js | 7 ++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/js/modules/loki_p2p_api.js b/js/modules/loki_p2p_api.js index 941495b10..cb22bbf6b 100644 --- a/js/modules/loki_p2p_api.js +++ b/js/modules/loki_p2p_api.js @@ -2,6 +2,8 @@ const EventEmitter = require('events'); +const offlinePingTime = 2 * 60 * 1000; // 2 minutes + class LokiP2pAPI extends EventEmitter { constructor(ourKey) { super(); @@ -85,6 +87,11 @@ class LokiP2pAPI extends EventEmitter { return; } clearTimeout(this.contactP2pDetails[pubKey].pingTimer); + this.contactP2pDetails[pubKey].pingTimer = setTimeout( + this.pingContact.bind(this), + offlinePingTime, + pubKey + ); this.contactP2pDetails[pubKey].isOnline = false; } diff --git a/libtextsecure/message_receiver.js b/libtextsecure/message_receiver.js index 985e42af3..8dcd2a80f 100644 --- a/libtextsecure/message_receiver.js +++ b/libtextsecure/message_receiver.js @@ -218,6 +218,9 @@ MessageReceiver.prototype.extend({ const promise = Promise.resolve(request.body.toArrayBuffer()) // textsecure.crypto .then(plaintext => { const envelope = textsecure.protobuf.Envelope.decode(plaintext); + if (isP2p) { + lokiP2pAPI.setContactOnline(envelope.source); + } // After this point, decoding errors are not the server's // fault, and we should handle them gracefully and tell the // user they received an invalid message @@ -945,9 +948,7 @@ MessageReceiver.prototype.extend({ return this.removeFromCache(envelope); }, handleDataMessage(envelope, msg) { - if (envelope.isP2p) { - lokiP2pAPI.setContactOnline(envelope.source); - } else { + if (!envelope.isP2p) { const timestamp = envelope.timestamp.toNumber(); const now = Date.now(); const ageInSeconds = (now - timestamp) / 1000; From 0516e69ff7a906e5a82944017f7d41e389b23a0f Mon Sep 17 00:00:00 2001 From: Beaudan Date: Mon, 18 Feb 2019 15:53:18 +1100 Subject: [PATCH 6/7] Fix tests --- js/modules/loki_p2p_api.js | 4 ++++ libloki/test/node/loki_p2p_api_test.js | 10 +++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/js/modules/loki_p2p_api.js b/js/modules/loki_p2p_api.js index cb22bbf6b..6475ccf9b 100644 --- a/js/modules/loki_p2p_api.js +++ b/js/modules/loki_p2p_api.js @@ -34,6 +34,10 @@ class LokiP2pAPI extends EventEmitter { pingTimer: null, isOnline: false, }; + if (isPing) { + this.setContactOnline(pubKey); + return; + } // Try ping this.pingContact(pubKey); return; diff --git a/libloki/test/node/loki_p2p_api_test.js b/libloki/test/node/loki_p2p_api_test.js index 5875927ea..5e52ba019 100644 --- a/libloki/test/node/loki_p2p_api_test.js +++ b/libloki/test/node/loki_p2p_api_test.js @@ -1,7 +1,7 @@ const { assert } = require('chai'); const LokiP2pAPI = require('../../../js/modules/loki_p2p_api'); -describe('LocalLokiServer', () => { +describe('LokiP2pAPI', () => { const usedKey = 'aPubKey'; const usedAddress = 'anAddress'; const usedPort = 'aPort'; @@ -64,16 +64,16 @@ describe('LocalLokiServer', () => { usedKey, usedAddress, usedPort, - true + false ); - assert.isTrue(this.lokiP2pAPI.isOnline(usedKey)); + assert.isFalse(this.lokiP2pAPI.isOnline(usedKey)); this.lokiP2pAPI.updateContactP2pDetails( usedKey, usedAddress, usedPort, - false + true ); - assert.isFalse(this.lokiP2pAPI.isOnline(usedKey)); + assert.isTrue(this.lokiP2pAPI.isOnline(usedKey)); }); it('Should set a contact as offline', () => { From 08dee148020f075cd150088a6235970fb9a747fa Mon Sep 17 00:00:00 2001 From: Beaudan Date: Thu, 21 Feb 2019 13:50:59 +1100 Subject: [PATCH 7/7] Review comment --- js/modules/loki_p2p_api.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/js/modules/loki_p2p_api.js b/js/modules/loki_p2p_api.js index 6475ccf9b..76b42484e 100644 --- a/js/modules/loki_p2p_api.js +++ b/js/modules/loki_p2p_api.js @@ -79,10 +79,7 @@ class LokiP2pAPI extends EventEmitter { isContactOnline(pubKey) { const contactDetails = this.contactP2pDetails[pubKey]; - if (!contactDetails || !contactDetails.isOnline) { - return false; - } - return contactDetails.isOnline; + return !!(contactDetails && contactDetails.isOnline); } setContactOffline(pubKey) {