Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#3499 closed change (fixed)

Create a clean messaging API for internal use

Reported by: trev Assignee: trev
Priority: P3 Milestone: Adblock-Plus-2.7.3-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: sebastian, sergz Blocked By: #3958
Blocking: #2401, #3851, #3853 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29338275/
https://codereview.adblockplus.org/29338279/
https://codereview.adblockplus.org/29338283/
https://codereview.adblockplus.org/29338417/
https://codereview.adblockplus.org/29338421/
https://codereview.adblockplus.org/29338425/

Description (last modified by trev)

Background

For our E10S support, we are currently using message manager directly. This results in overly complicated code however: message names have to avoid conflicts with other extensions, message handlers have to be removed explicitly, response handling has to be implemented explicitly, messaging code is duplicated for the first-run page.

What to change

Create a clean internal API, accessible in both parent and child via the messaging module:

  • Port constructor binds a communication port to a message manager.
  • port instance is the default port (communicating with the parent for process scripts, broadcasting to all children for the parent).
  • Port.disconnect() shuts down the port (done automatically for the default port when the extension is shut down).
  • Port.on() adds a handler for a particular message (message names are specific to Adblock Plus and won't conflict with other extensions).
  • Port.off() removes a handler.
  • Port.emit() sends/broadcasts an asynchronous message.
  • Port.emitWithResponse() sends/broadcasts an asynchronous message. It returns a promise that will be resolved when the response is received.
  • Port.emitSync() sends a synchronous message, response is returned from the function.

Message handlers have two parameters: payload (the data sent by the sender) and sender (a Port instance). The handler has the choice between returning undefined (no response), a value (actual response) or a promise (indicates an async response). We should warn if multiple handlers choose to return a value for the same message or if a handler chooses to return a promise for a sync message.

Change History (23)

comment:1 Changed 3 years ago by trev

  • Description modified (diff)

comment:2 Changed 3 years ago by trev

  • Description modified (diff)

comment:3 Changed 3 years ago by trev

  • Ready set

comment:4 Changed 3 years ago by sebastian

  • Cc sebastian added

comment:5 in reply to: ↑ description Changed 3 years ago by sebastian

Replying to trev:and sendResponse (callback to be called with the response).

What do we need that callback for if the return value can be the result or a promise that provides the result?

Version 0, edited 3 years ago by sebastian (next)

comment:6 follow-up: Changed 3 years ago by trev

  • Description modified (diff)

True, I forgot to remove that parameter.

Actually, there is a complication I didn't consider. When the parent is broadcasting a message, there will normally be two child processes receiving it. How is the parent going to know that it received the response if each child process is going to send one? It seems that the parent needs to wait for two responses (more precisely, nsIMessageBroadcaster.childCount) to arrive, then choose the one that isn't undefined (also warn if more than one is).

comment:7 Changed 3 years ago by sebastian

I suppose it can bail out as soon as it gets a non-undefined response. Things probably get a little more complicated with promises though.

comment:8 Changed 3 years ago by trev

That's not going to make things simpler - even if all responses are undefined it still has to recognize that the message has been processed and the callback can be removed. Also, collecting all responses allows warning properly if multiple processes decide to respond (indicates a bug). So that's what I implemented now.

comment:9 Changed 3 years ago by sebastian

Well, it would still be more efficient in the average case with multiple message responders, if we already call the message listener as soon as a response arrived. We can still log an error if later another response arrived.

comment:10 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by sergz

  • Cc sergz added

I would like to share a fresh view on the API without looking at the real code.

  1. regarding Port.emit:

What about returning of an array of promises? In that case it seems the caller of Port.emit can decide and easily express what he wants using Promise API, e.g. results of resolving of all promises or when at least one is rejected (Promise.all), or when only one result is resolved or at least one promise is rejected (Promise.race).
We still can add some post-action to each promise for debug, for instance.

As I understand from the description Port is supposed to be used either in parent process or in client process. Another thing here is that we don't know how many listeners are in chrome process or in parent process, and they can "send response" not in the order of receiving of the message, which can result in a really tricky race condition bugs. So we cannot actually always use nsIMessageBroadcaster.childCount and cannot rely on the order of the response messages.

  1. if we see that we often need to wait only for first non-undefined resolved value or we are expecting to have only one promise in that array then we can generalize it by creating a helper function (and extend Promise API) or by adding a function with the more specific name into Port.prototype. The implementation of such functions seems quite straightforward. Actually, waiting for a first non-undefined value can result in some bugs because of a race conditions.
  1. regarding Port.emitSync. I understand that it can be very convenient and I have no objections regarding it now, but do we actually need it, especially in Chrome process? May be it's a signal to improve something, so we avoid possible unnecessary blocking.

BTW, speaking about efficiency, does it really makes sense to try to optimize something here now? Just wonder what are the cases when we expect to have big arrays and chains of promises?

So, I would propose to think rather on less generalization, may be different API for Port in Chrome and Content process, don't complicate the logic in Port.emit and Port.emitSync. The class by itself only from the description looks already extremely helpful, try to use it and come later with the necessary logic or optimizations improvements.

comment:11 in reply to: ↑ 10 Changed 3 years ago by trev

Replying to sergz:

  1. regarding Port.emit:

What about returning of an array of promises?

There are two drawbacks here:

1) Complexity. As it is now, we don't have 1:n communication anywhere, at least when a response is expected. It's always 1:1 communication, and we broadcast merely because the parent doesn't know which child is going to respond. Right now the framework will select the one "actual" response and this simplifies the callers a lot.

2) Performance. If we return promises then we cannot know whether the caller actually expects a response. Currently, we won't even ask for a response if the caller doesn't provide a callback (meaning: no call tracking overhead, no response messages sent). And that's the majority of calls.

So we cannot actually always use nsIMessageBroadcaster.childCount and cannot rely on the order of the response messages.

Right now we can because the messaging module will already choose one response when the message is processed in the child - only one response is sent back to parent, not all of them.

  1. regarding Port.emitSync. I understand that it can be very convenient and I have no objections regarding it now, but do we actually need it, especially in Chrome process?

Yes, we need it. And - yes, the JSDoc comment currently says "DO NOT USE UNLESS ABSOLUTELY NECESSARY." And it is absolutely necessary when it comes to content policies, these expect a synchronous response right now. I hope that Mozilla will fix this eventually. Chrome doesn't have that issue and there we wouldn't need it (not that we can implement it at all there).

BTW, speaking about efficiency, does it really makes sense to try to optimize something here now?

Not too much I think.

comment:12 Changed 3 years ago by sebastian

I agree, that we rather shouldn't support multiple responses, for reasons already mentioned. However, I like the idea of Port.emit() returning a promise. But I also see that this will add extra costs in cases where no response is expected. So how about providing two methods, one that returns a promise, and one that returns nothing.

comment:13 Changed 3 years ago by trev

  • Description modified (diff)

Ok, I added Port.emitWithResponse() to the description, returning a promise.

comment:14 Changed 3 years ago by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:15 Changed 3 years ago by trev

  • Review URL(s) modified (diff)

comment:16 Changed 3 years ago by trev

  • Blocking 2401 added

comment:17 Changed 3 years ago by trev

  • Blocking 3851 added

comment:18 Changed 3 years ago by trev

  • Blocking 3853 added

comment:19 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockplus/rev/0d68dbc94bee

comment:21 Changed 3 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:22 Changed 3 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Both first-run and subscribe functionality are still working as expected in both non-E10S and E10S Firefox after these changes.

ABP 2.7.2.4163
Firefox 38 / 44 / Windows 8
Nightly 48.0a1 (2016-04-19) / Windows 8
Firefox 38 / 44 / OS X 10.11
Firefox 45.01 / Ubuntu 14.04
Nightly 48.0a1 (2016-04-19) / Ubuntu 14.04

comment:23 Changed 3 years ago by trev

  • Blocked By 3958 added
Note: See TracTickets for help on using tickets.