Issue 3499 - Create a clean messaging API for internal use

lib/messaging.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 const MESSAGE_NAME = "AdblockPlus:Message";
 const RESPONSE_NAME = "AdblockPlus:Response";
 
+function isPromise(value)
+{
+  // value instanceof Promise won't work - there can be different Promise
+  // classes (e.g. in different contexts) and there can also be promise-like
+  // classes (e.g. Task).
+  return (value && typeof value.then == "function");
+}
+
 function sendMessage(messageManager, messageName, payload, callbackID)
 {
   let request = {messageName, payload, callbackID};
   if (messageManager instanceof Ci.nsIMessageSender)
   {
     messageManager.sendAsyncMessage(MESSAGE_NAME, request);
     return 1;
   }
   else if (messageManager instanceof Ci.nsIMessageBroadcaster)
   {
     messageManager.broadcastAsyncMessage(MESSAGE_NAME, request);
     return messageManager.childCount;
   }
   else
   {
     Cu.reportError("Unexpected message manager, impossible to send message");
     return 0;
   }
 }
 
 function sendSyncMessage(messageManager, messageName, payload)
 {
   let request = {messageName, payload};
   let responses = messageManager.sendRpcMessage(MESSAGE_NAME, request);
+  let processor = new ResponseProcessor(messageName);
   for (let response of responses)
-    if (typeof response != "undefined")
-      return response;

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: What's the reason for not using `flattenResponses()` here?

[Wladimir Palant, 2016/03/21 15:31:31]

No real reason, flattenResponses() was simply introduced after the function
here. Done.

-  return undefined;
-}
-
-function flattenResponses(responses, messageName)
-{
-  let result = undefined;
-  for (let response of responses)
+    processor.add(response);
+  return processor.value;
+}
+
+function ResponseProcessor(messageName)
+{
+  this.value = undefined;
+  this.add = function(response)
   {
     if (typeof response == "undefined")

[Erik, 2016/03/17 05:02:04]

nit `==` -> `===`

[Wladimir Palant, 2016/03/21 15:31:32]

We generally don't use === unless it makes a difference.

-      continue;
-
-    if (typeof result == "undefined")

[Erik, 2016/03/17 05:02:04]

nit `==` -> `===`

[Wladimir Palant, 2016/03/21 15:31:33]

Same as above.

-      result = response;
+      return;
+
+    if (typeof this.value == "undefined")
+      this.value = response;
     else
       Cu.reportError("Got multiple responses to message '" + messageName + "', only first response was accepted.");
-  }
-  return result;
+  };
 }
 
 function getSender(origin)
 {
   if (origin instanceof Ci.nsIDOMXULElement)
     origin = origin.messageManager;
 
   if (origin instanceof Ci.nsIMessageSender)
     return new LightWeightPort(origin);
   else
@@ skipping 55 matching lines @@
   {
     this._messageManager.removeMessageListener(MESSAGE_NAME, this._handleRequest);
     this._messageManager.removeMessageListener(RESPONSE_NAME, this._handleResponse);
   },
 
   _sendResponse: function(sender, callbackID, payload)
   {
     if (!sender || typeof callbackID == "undefined")
       return;
 
-    var response = {callbackID, payload};

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: Unless there's a particular reason not to, please use `let` here like in
the rest of this file.

[Wladimir Palant, 2016/03/21 15:31:33]

Done.

+    let response = {callbackID, payload};
     sender._messageManager.sendAsyncMessage(RESPONSE_NAME, response);
   },
 
   _handleRequest: function(message)
   {
     let sender = getSender(message.target);
     let {callbackID, messageName, payload} = message.data;
 
     let result = this._dispatch(messageName, payload, sender);
-    if (result && typeof result.then == "function")

[Erik, 2016/03/17 05:02:04]

It'd be nice to have a `isPromise` function to reuse, I think it might make this
bit of code a little easier to read.

[Thomas Greiner, 2016/03/17 12:09:26]

You mean `result instanceof Promise`?

[Wladimir Palant, 2016/03/21 15:31:33]

No, `result instanceof Promise` would be wrong - there isn't necessary a single
Promise class (shim/native + different frames), and there can also be
promise-like classes, e.g. Task.

I added a function isPromise().

+    if (isPromise(result))
     {
       // This is a promise - asynchronous response
       if (message.sync)
       {
         Cu.reportError("Asynchronous response to the synchronous message '" + messageName + "' is not possible");
         return undefined;
       }
 
-      result.then(result => {
+      result.then(result =>
+      {
         this._sendResponse(sender, callbackID, result)
-      }, e => {
+      }, e =>
+      {
         Cu.reportError(e);
         this._sendResponse(sender, callbackID, undefined);
       });
     }
     else
       this._sendResponse(sender, callbackID, result);
 
     return result;
   },
 
   _handleResponse: function(message)
   {
     let {callbackID, payload} = message.data;
-    if (!this._responseCallbacks.has(callbackID))
+    let callbackData = this._responseCallbacks.get(callbackID);
+    if (!callbackData)
       return;
 
-    let [callback, messageName, responses, expectedResponses] =
-        this._responseCallbacks.get(callbackID);
-    responses.push(payload);

[Thomas Greiner, 2016/03/17 12:09:26]

Why do you wait for and store all the responses if you're throwing all but one
of them away anyway?

[Wladimir Palant, 2016/03/21 15:31:31]

Because otherwise this code gets rather complicated, at least if I still want to
warn about multiple responses.

[Thomas Greiner, 2016/03/21 18:43:22]

So decrementing `expectedResponses` is not an option or do you mean that you
intend to access individual responses in a later change?

[Wladimir Palant, 2016/03/21 20:42:34]

Without storing all the values in an array we cannot call flattenResponses and
reuse the code.

Anyway, I solved this by using a generator-like object. I used a proper
generator first but the generator protocol is too complicated for this scenario.

-    if (responses.length == expectedResponses)

[Erik, 2016/03/17 05:02:04]

nit `==` -> `===`

[Wladimir Palant, 2016/03/21 15:31:32]

Same as above.

+    let [callback, processor, expectedResponses] = callbackData;
+
+    try
+    {
+      processor.add(payload);
+    }
+    catch (e)
+    {
+      Cu.reportError(e);
+    }
+
+    callbackData[2] = --expectedResponses;
+    if (expectedResponses <= 0)
     {
       this._responseCallbacks.delete(callbackID);
-      callback(flattenResponses(responses, messageName));
+      callback(processor.value);
     }
   },
 
   _dispatch: function(messageName, payload, sender)
   {
     let callbacks = this._callbacks.get(messageName);
     if (!callbacks)
       return undefined;
 
     callbacks = callbacks.slice();

[Thomas Greiner, 2016/03/17 12:09:26]

Why do you copy the array? I don't see that a callback would modify the array
while iterating through it.

[Wladimir Palant, 2016/03/21 15:31:32]

It sure could. Classic example is a callback that removes itself when called.
This works fine as long it's the only callback. However, if there is a second
callback in the list it will be skipped. We actually hit that exact issue for
other code not too long ago.

[Thomas Greiner, 2016/03/21 18:43:22]

Of course, in theory it could happen because it's not a "private" variable.
However, that'd require other code to access `_callbacks` from outside which
it's not supposed to do. Do you think that convention is insufficient here?

[Wladimir Palant, 2016/03/21 20:42:34]

No, it would merely have to call port.off(arguments.callee). This would modify
the list of callbacks right under our feet.

[Thomas Greiner, 2016/03/22 11:15:07]

Ah, right. Nevermind then.

-    let responses = [];
+    let processor = new ResponseProcessor(messageName);
     for (let callback of callbacks)
     {
       try
       {
-        responses.push(callback(payload, sender));
+        processor.add(callback(payload, sender));
       }
       catch (e)
       {
         Cu.reportError(e);
       }
     }
-    return flattenResponses(responses, messageName);
+    return processor.value;
   },
 
   /**
    * Function to be called when a particular message is received
    * @callback Port~messageHandler
    * @param payload data attached to the message if any
    * @param {LightWeightPort} sender object that can be used to communicate with

[Erik, 2016/03/17 05:02:05]

this param appears to just accept a function, and I don't think it would accept
a `LightWeightPort`.

[Wladimir Palant, 2016/03/21 15:31:32]

Explanation below.

    *      the sender of the message, could be null
    * @return the handler can return undefined (no response), a value (response
    *      to be sent to sender immediately) or a promise (asynchronous
    *      response).

[Erik, 2016/03/17 05:02:05]

`on` doesn't appear to return a promise, or anything.

[Wladimir Palant, 2016/03/21 15:31:32]

Explanation below.

    */
 
   /**
    * Adds a handler for the specified message.
    * @param {string} messageName message that would trigger the callback
    * @param {Port~messageHandler} callback

[Erik, 2016/03/17 05:02:04]

same comment as above, this appears to just accept a function, also I'm not sure
why this function appears to have two sets of docs.

[Wladimir Palant, 2016/03/21 15:31:32]

Please see http://usejsdoc.org/tags-callback.html - the first block documents
the callback type.

    */
   on: function(messageName, callback)
   {
-    if (!this._callbacks.has(messageName))
-      this._callbacks.set(messageName, []);

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: Since you're using `Map` I wonder why you're not using `Set` to avoid
having to deal with array indices.

[Wladimir Palant, 2016/03/21 15:31:31]

As Sebastian pointed out, Node.js (unlike Add-on SDK) allows duplicate
callbacks. I changed the logic here to be consistent.

[Thomas Greiner, 2016/03/21 18:43:23]

Interesting, I wonder why they thought that'd be a good idea…

[Wladimir Palant, 2016/03/21 20:42:34]

Probably because it's simpler/more efficient to implement. Also, arguably
eliminating duplicate callbacks tends to mask programming errors.

-
     let callbacks = this._callbacks.get(messageName);
-    if (callbacks.indexOf(callback) < 0)
+    if (callbacks)
       callbacks.push(callback);
+    else
+      this._callbacks.set(messageName, [callback]);
   },
 
   /**
    * Removes a handler for the specified message.
    * @param {string} messageName message that would trigger the callback
    * @param {Port~messageHandler} callback

[Erik, 2016/03/17 05:02:04]

if I am not mistaken about the above comments for `on`, then this should just be
`function` as well.

[Wladimir Palant, 2016/03/21 15:31:32]

Same as above.

    */
   off: function(messageName, callback)
   {
     let callbacks = this._callbacks.get(messageName);
     if (!callbacks)
       return;
 
     let index = callbacks.indexOf(callback);
     if (index >= 0)
       callbacks.splice(index, 1);
   },
 
   /**
    * Sends a message.
    * @param {string} messageName message identifier
    * @param [payload] data to attach to the message

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: Is this valid JSDoc syntax even with the type missing?

[Wladimir Palant, 2016/03/21 15:31:32]

Yes, it is.

    */
   emit: function(messageName, payload)
   {
     sendMessage(this._messageManager, messageName, payload, undefined);
   },
 
   /**
    * Sends a message and expects a response.
    * @param {string} messageName message identifier
    * @param [payload] data to attach to the message
    * @return {Promise} promise that will be resolved with the response
    */
   emitWithResponse: function(messageName, payload)
   {
     let callbackID = ++this._responseCallbackCounter;
     let expectedResponses = sendMessage(
         this._messageManager, messageName, payload, callbackID);
-    return new Promise((resolve, reject) => {

[Erik, 2016/03/17 05:02:04]

nit add a newline here?

[Wladimir Palant, 2016/03/21 15:31:32]

Done.

+    return new Promise((resolve, reject) =>
+    {
       this._responseCallbacks.set(callbackID,
-          [resolve, messageName, [], expectedResponses]);
+          [resolve, new ResponseProcessor(messageName), expectedResponses]);
     });
   },
 
   /**
    * Sends a synchonous message (DO NOT USE unless absolutely unavoidable).
    * @param {string} messageName message identifier
    * @param [payload] data to attach to the message
    * @return response returned by the handler
    */
   emitSync: function(messageName, payload)
@@ skipping 12 matching lines @@
 catch (e)
 {
   // Parent
   messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
                      .getService(Ci.nsIMessageListenerManager);
 }
 
 let port = new Port(messageManager);
 onShutdown.add(() => port.disconnect());
 exports.port = port;