Page MenuHomePhabricator

Multiplex AJAX calls
ClosedPublic

Authored by joshuaspence on Jan 13 2015, 8:03 AM.

Details

Summary

Fixes T5344. Essentially, we only make the AJAX request to /notification/individual/ if we are the leader tab (i.e. only one tab will make this request). Once a response has been received from the server (containing the contents of the notification), we broadcast the message contents back to all other tabs for rendering.

Test Plan

Opened two tabs on /notification/status/ and clicked "Send Test Notification".

Before

tail -f /var/log/phabricator-access.log | grep /notification/individual/
[Tue, 13 Jan 2015 20:10:37 +1100]   17033   phabricator 10.0.0.1    josh    PhabricatorNotificationIndividualController -   /notification/individual/-200   236036
[Tue, 13 Jan 2015 20:10:37 +1100]   17657   phabricator 10.0.0.1    josh    PhabricatorNotificationIndividualController -   /notification/individual/-200   24130

After

tail -f /var/log/phabricator-access.log | grep /notification/individual/
[Tue, 13 Jan 2015 20:11:15 +1100]   17657   phabricator 10.0.0.1    josh    PhabricatorNotificationIndividualController -   /notification/individual/-200   180217

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Multiplex AJAX calls.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence edited the test plan for this revision. (Show Details)
epriestley edited edge metadata.

This looks good to me, but some structural feedback inline. Does that stuff seem reasonable? The handler/renderer split feels "application-ey" to me, although I think my objection is pretty subjective.

When I looked through this at first, I was thinking we should short-circuit the behavior when we receive a message from the server (i.e., not broadcast it), but the more I think about it the more I like this approach. We'll send every tab a message and currently they'll all ignore it, but localSocket seems really fast, and this is a lot more general and cleaner than special-casing the websocket messages.

webroot/rsrc/js/application/aphlict/Aphlict.js
119–123

This feels kind of too-coupled to me.

In particular, there's a lack of symmetry between the handler (which just calls JX.invoke) and the renderer (which has a ton of application logic).

I think it would be cleaner to make this a default: case, call the handler, and pass it the message and the is_leader flag.

Then the handler would look like:

switch (message.type) {
  case 'aphlict.server':
    JX.Stratcom.invoke('aphlict-receive-message'); // maybe rename to aphlict-server-message?
    break;
  case 'notification.individual':
    JX.Stratcom.invoke('aphlict-notification-message');
    break;
}

Basically, handle protocol logic here but delegate all application logic to the handler callback.

webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js
26–28

With the adjustments above, we could also make is_leader available in these listeners, which would simplify them a bit (they wouldn't need to use a callback here).

70

This would become JX.Stratcom.listen('aphlict-notification-message', ...) or whatever.

This revision now requires changes to proceed.Jan 14 2015, 2:20 PM

Thanks, useful feedback. My JavaScript isn't great and I've never really used Javelin before.

joshuaspence edited edge metadata.

Made requested changes (still need to retest this)

epriestley edited edge metadata.

Woah, this ended up really clean after separating all the minor stuff into D11398

webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js
26–29

Not a big deal, but it would be vaguely nice to pass is_leader through here eventually to avoid this callback. I don't think it's worth including in this diff, but maybe the next time we touch this stuff.

This revision is now accepted and ready to land.Jan 15 2015, 2:14 PM

One minor issue with this approach (although theoretical) is that the AJAX request from the leader tab will fail. Previously, if one of the AJAX requests to /notification/individual/ failed then all other tabs would still receive and render the notification (assuming that they were able to make a successful request to /notification/individual/). I guess that this type of failure should be handled by JX.Router, but I'm not too familiar with Javelin.

This revision was automatically updated to reflect the committed changes.