Page MenuHomePhabricator

Modify the Aphlict client to use `LocalConnection`.
ClosedPublic

Authored by joshuaspence on May 29 2014, 7:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 29, 2:51 PM
Unknown Object (File)
Mon, Apr 29, 9:41 AM
Unknown Object (File)
Wed, Apr 24, 10:17 PM
Unknown Object (File)
Fri, Apr 19, 3:37 AM
Unknown Object (File)
Fri, Apr 19, 3:37 AM
Unknown Object (File)
Thu, Apr 11, 8:41 AM
Unknown Object (File)
Thu, Apr 11, 4:50 AM
Unknown Object (File)
Sat, Apr 6, 12:21 AM
Subscribers

Details

Summary

Ref T4324. Currently, an Aphlict client (with a corresponding connection to the Aphlict Server) is created for every tab that a user has open. This significantly affects the scalability of Aphlict as a service. Instead, we can use LocalConnection instances to coordinate the communication of multiple Aphlict clients to the server.

Similar functionality existed prior to D2704, but was removed as the author was not able to get this functionality working as intended. It seems that the main issue with the initial attempt was the use of the setTimeout function, which seemed to be a blocking call which prevented messages from being received. I have instead used an event-based model using a Timer object.

Roughly this works as follows:

  1. The first instance will create an AphlictClient and an AphlictMaster. The AphlictClient will register itself with the AphlictMaster and will consequently be notified of incoming messages.
  2. The AphlictClient is then responsible for pinging the AphlictMaster at regular intervals. If the client does not ping the master in a given period of time, the master will assume that the client is dead and will remove the client from the pool.
  3. Similarly, the AphlictMaster is required to respond to pings with a "pong" response. The pong response lets the clients know that the AphlictMaster is still alive. If the clients do not receive a pong in a given period of time, then the clients will attempt to spawn a new master.
Test Plan

I have tested this on our Phabricator install with a few tabs opened and inspecting the console output. I will upload a screencast of my test results.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Modify the Aphlict client to use `LocalConnection`..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
support/aphlict/client/build_aphlict_client.sh
12

This doesn't seem to work properly (at all) in /bin/sh.

16–26

This cd and mv stuff is overkill... just do it one command.

support/aphlict/client/src/AphlictClient.as
27

There's nothing special about this constant, I just picked something that seemed reasonable.

Generally, we can decrease this value and the clients should be able to react faster to loosing the AphlictMaster. This would, however, come at the cost of increased CPU usage.

epriestley edited edge metadata.

One minor thing -- let's put a layer of indirection on top of console.log so that this doesn't spam the console up by default.

This seems to work well otherwise. Thanks!

support/aphlict/client/build_aphlict_client.sh
18

(At some point, we should define debug and non-debug builds for this. Maybe moving it to bin/aphlict build or similar would make sense.)

support/aphlict/client/src/Aphlict.as
34–35

Let's pass this via something like JX.Aphlict.didReceiveLog (a new function this diff would add) so we can enable it only when debugging is enabled? The current aphlict.swf doesn't actually log anything, so it doesn't spam up the console.

This revision now requires changes to proceed.May 29 2014, 1:29 PM
In D9327#9, @epriestley wrote:

One minor thing -- let's put a layer of indirection on top of console.log so that this doesn't spam the console up by default.

This seems to work well otherwise. Thanks!

I was actually planning on removing the console logging entirely... Or should I leave it there but use something like JX.Aphlict.didReceiveLog instead so that we can turn it on and off from the JavaScript?

Yeah, I generally think we should move to expose more information about notifications in debug mode. They're somewhat hard to configure right now and they're also hard to support over IRC (users will just be like "it dun work" and I don't have too many tools to help).

joshuaspence edited edge metadata.
  • Conditional logging with didReceiveLog
  • Made some functions protected
  • Various changes to log messages.

Hmm... The console logging doesn't seem to work as I expect.

> __DEV__ = true
true

[After a notification is triggered]
Uncaught Error: JX.Notification.invoke("close", ...): invalid event type. Valid event types are: . core.pkg.js:2
JX.$E core.pkg.js:2
proto.invoke core.pkg.js:67
JX.install.statics._hide core.pkg.js:768
JX.install.members.hide core.pkg.js:765
joshuaspence edited edge metadata.
  • Fix didReceiveLog

Looking at the code, maybe the cleaner approach would be to raise these as a normal event (with type log or similar).

The actual flag that should be used is config.debug in the aphlict-listen behavior (which is derived from notification.debug in Phabricator's config). The __DEV__ constant is sort of correlated, but notification.debug is a much better flag to use.

However, it's not readily available at the JX.Aphlict level. If these events are just normal events with type log, it could be handled in onaphlictmessage() in the aphlict-listen behavior (and will be automatically, I think).

Well, it will be handled automatically by being shown as a bubble. That might be annoying, but we could special case it if it is. I don't think it's terrible to start out with logs going to bubbles, for now.

  • Pass console messages as an event
epriestley edited edge metadata.

Cool. Let's see how this works. Thanks!

This revision is now accepted and ready to land.May 29 2014, 2:03 PM

I still can't figure out how to make the logging work :(

epriestley updated this revision to Diff 22219.

Closed by commit rPa42ec32c98b6 (authored by @joshuaspence, committed by @epriestley).

I still can't figure out how to make the logging work :(

Nevermind... it does seem to work. I do still get Uncaught Error: JX.Routable.listen("start", ...): invalid event type. Valid event types are: . in the console, but this must be unrelated.

Enable notification.debug in your config? Works fine for me:

Screen_Shot_2014-05-29_at_7.05.52_AM.png (1×1 px, 182 KB)

You'll also need phabricator.developer-mode on, possibly, depending on some other configuration.

(If stuff is being minified with jsxmin in production mode (which is complicated and under/un-documented, but maybe you've been ambitious in configuring this) the __DEV__ blocks will be completely removed from the minified version of the Javascript, so changing __DEV__ at runtime won't have an effect. It's unlikely this is the issue, though.)

Enable notification.debug in your config? Works fine for me:

Screen_Shot_2014-05-29_at_7.05.52_AM.png (1×1 px, 182 KB)

You'll also need phabricator.developer-mode on, possibly, depending on some other configuration.

(If stuff is being minified with jsxmin in production mode (which is complicated and under/un-documented, but maybe you've been ambitious in configuring this) the __DEV__ blocks will be completely removed from the minified version of the Javascript, so changing __DEV__ at runtime won't have an effect. It's unlikely this is the issue, though.)

Possibly a stupid question, but am I meant to set __DEV__ = true in the console? Or is there a proper way to do this?

In general: __DEV__ should be set automatically, based on the phabricator.developer-mode setting. You should not need to set it from the console.

Specifically:

  • __DEV__ is defined directly, in PhabricatorBarePageView, based on the value of phabricator.developer-mode. If you "View Source", you should see the definition near the top of the page in the headers.
  • When Javascript is minified with jsxmin (a complex, purpose-built minifier), __DEV__ is evaluated as 0 and trivially unreachable branches are stripped. This means that if (__DEV__) { ... } blocks are completely removed by the minifier. This is the primary purpose of the __DEV__ flag: it marks blocks for total removal in production JS builds.
  • Since building jsxmin is complicated and the benefits (slightly smaller JS wire size) aren't huge (we fall back to a less sophisticated minifier if jsxmin is not available), we don't currently expect installs to build or configure jsxmin. I don't think we even have it running on this server right now. In these cases, __DEV__ is just equivalent to phabricator.developer-mode.

In general: __DEV__ should be set automatically, based on the phabricator.developer-mode setting. You should not need to set it from the console.

Yep, that got it. Thanks.