Page MenuHomePhabricator
Differential D9458

Modify the Aphlict server to transmit messages instead of broadcasting them.
ClosedPublic
Actions

Authored by joshuaspence on Jun 10 2014, 7:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 9:07 PM
Unknown Object (File)
Fri, Dec 13, 9:14 AM
Unknown Object (File)
Wed, Dec 11, 6:07 AM
Unknown Object (File)
Mon, Dec 9, 1:31 PM
Unknown Object (File)
Nov 10 2024, 2:04 AM
Unknown Object (File)
Oct 24 2024, 9:30 PM
Unknown Object (File)
Oct 24 2024, 9:30 PM
Unknown Object (File)
Oct 24 2024, 9:30 PM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T5284: Allow users to receive Aphlict notifications for an object they are viewing
T4324: Change notification server so it only alerts appropriate clients
Commits
Restricted Diffusion Commit
rP84d259cea223: Modify the Aphlict server to transmit messages instead of broadcasting them.
Summary

Fixes T4324. Ref T5284. Currently, Aphlict notifications are broadcast to all clients. The client is then responsible for determining whether or not the notification is relevant (specifically, it fires off an AJAX request to /notification/individual/). This is massive overkill and results in a large number of requests. In particular, all tabs on a client machine will each fire off individual AJAX requests.

Instead, transmit notifications only to the relevant clients. Clients are responsible for subscribing to PHIDs that they are interested in. The server will only transmit notifications to clients that have subscribed to, at least one of, the related PHIDs.

Test Plan

I opened up two clients on the same host (incognito tabs in Chrome). Here is the output from the server:

> sudo ./bin/aphlict debug
Starting Aphlict server in foreground...
Launching server:

    $ 'nodejs' '/usr/src/phabricator/src/applications/aphlict/management/../../../../support/aphlict/server/aphlict_server.js' --port='22280' --admin='22281' --host='localhost' --user='aphlict'

[Wed Jun 11 2014 19:10:27 GMT+0000 (UTC)] Started Server (PID 4546)
[Wed Jun 11 2014 19:10:36 GMT+0000 (UTC)] <FlashPolicy> Policy Request From ::ffff:192.168.1.1
[Wed Jun 11 2014 19:10:37 GMT+0000 (UTC)] <Listener/1> Connected from ::ffff:192.168.1.1
[Wed Jun 11 2014 19:10:37 GMT+0000 (UTC)] <Listener/1> Received data: {"command":"subscribe","data":["PHID-USER-cb5af6p4oepy5tlgqypi"]}
[Wed Jun 11 2014 19:10:37 GMT+0000 (UTC)] <Listener/1> Subscribed to: ["PHID-USER-cb5af6p4oepy5tlgqypi"]
[Wed Jun 11 2014 19:10:39 GMT+0000 (UTC)] <Listener/1> Received data: {"command":"subscribe","data":["PHID-USER-kfohe3ca5oe6ygykmioq"]}
[Wed Jun 11 2014 19:10:39 GMT+0000 (UTC)] <Listener/1> Subscribed to: ["PHID-USER-kfohe3ca5oe6ygykmioq"]
[Wed Jun 11 2014 19:10:42 GMT+0000 (UTC)] notification: {"key":"6023751084283587681","type":"notification","subscribers":["PHID-USER-cb5af6p4oepy5tlgqypi"]}
[Wed Jun 11 2014 19:10:42 GMT+0000 (UTC)] <Listener/1> Wrote Message

I verified (using the "Network" tab in Chrome) that an AJAX request to /notification/individual/ was only made in the tab belonging to the user that triggered the test notification.

Diff Detail

Repository
rP Phabricator
Branch
aphlict-client
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 970
Build 970: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 22578.Jun 10 2014, 7:19 PM
joshuaspence retitled this revision from to Modify the Aphlict server to transmit messages instead of broadcasting them..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence added tasks: T4324: Change notification server so it only alerts appropriate clients, T5284: Allow users to receive Aphlict notifications for an object they are viewing.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 10 2014, 7:19 PM
Herald added subscribers: Korvin, epriestley. · View Herald Transcript
Harbormaster completed remote builds in B965: Diff 22578.Jun 10 2014, 7:21 PM
joshuaspence updated this revision to Diff 22579.Jun 10 2014, 7:23 PM
joshuaspence edited edge metadata.
  • Implement the subscribe method.
  • Implement the unsubscribe method.
Harbormaster completed remote builds in B966: Diff 22579.Jun 10 2014, 7:24 PM
epriestley requested changes to this revision.Jun 10 2014, 7:45 PM
epriestley edited edge metadata.

This looks good, the only real issue I see is the that I think we'll only find JSON message boundaries through acts of luck right now (see inline). Using a length + JSON scheme like we do when transmitting would resolve this.

I think some code can be simplified (and get improved runtime for large N) by using objects as hashes instead of arrays, but that's minor.

support/aphlict/server/aphlict_server.js
15

What's this?

86–87

I'd expect us to need to read lengths and parse messages off this stream? That is, this presumably only works by luck/accident/messages all being smaller than one packet right now?

205–207

(This is reasonable for now, but it would be vaguely nice to maintain a map eventually.)

support/aphlict/server/lib/AphlictListener.js
6–7

I'd expect this._subscriptions = {} here?

20–24

Consider this._subscriptions[phid] = true;

33–37

Consider delete this._subscriptions[phid];

43

Consider isSubscribedToAny() or similar, for clarity.

43–48

Consider ... if (phid in this._subscriptions) { return true; } ...

This revision now requires changes to proceed.Jun 10 2014, 7:45 PM
joshuaspence updated this revision to Diff 22581.Jun 10 2014, 8:08 PM
joshuaspence edited edge metadata.
  • Remove a dead require
  • Allow data to be of an arbitrary size
  • Use an object instead of an array
Harbormaster completed remote builds in B967: Diff 22581.Jun 10 2014, 8:09 PM
epriestley added a comment.Jun 10 2014, 8:18 PM

Cool, this all seems pretty sensible to me.

support/aphlict/server/aphlict_server.js
91

Probably needs a return; here if length is less than 2.

92

Consider using 8-character padded ASCII text for consistency with transmissions and readability.

98–99

Probably needs return;.

support/aphlict/server/lib/AphlictListener.js
20–24

You could do this assignment/delete unconditionally if you want, they won't hurt anything.

joshuaspence updated this revision to Diff 22587.Jun 10 2014, 8:33 PM
joshuaspence edited edge metadata.
  • Need to read more bytes
  • Buffer.concat is a static function
  • Process data in a loop
  • Be less conditional
joshuaspence added a comment.Jun 10 2014, 8:33 PM

I think that this is basically ready (from the server side at least). Here is some debug output (I just had the clients sending some dummy data):

sudo ./bin/aphlict debug
Starting Aphlict server in foreground...
Launching server:

    $ 'nodejs' '/usr/src/phabricator/src/applications/aphlict/management/../../../../support/aphlict/server/aphlict_server.js' --port='22280' --admin='22281' --host='localhost' --user='aphlict'

[Tue Jun 10 2014 20:31:08 GMT+0000 (UTC)] Started Server (PID 2325)
[Tue Jun 10 2014 20:31:10 GMT+0000 (UTC)] <FlashPolicy> Policy Request From ::ffff:10.0.0.1
[Tue Jun 10 2014 20:31:10 GMT+0000 (UTC)] <Listener/1> Connected from ::ffff:10.0.0.1
[Tue Jun 10 2014 20:31:10 GMT+0000 (UTC)] <Listener/1> Received data: {"command":"subscribe","data":["foo","bar"]}
[Tue Jun 10 2014 20:31:10 GMT+0000 (UTC)] <Listener/1> Subscribed to: ["foo","bar"]
[Tue Jun 10 2014 20:31:10 GMT+0000 (UTC)] <Listener/1> Received data: {"command":"unsubscribe","data":["bar"]}
[Tue Jun 10 2014 20:31:10 GMT+0000 (UTC)] <Listener/1> Unsubscribed from: ["bar"]
Harbormaster completed remote builds in B969: Diff 22587.Jun 10 2014, 8:34 PM
joshuaspence updated this revision to Diff 22590.Jun 10 2014, 8:54 PM

Add client side support

joshuaspence added a comment.Jun 10 2014, 8:55 PM

(This doesn't exactly work yet, but this is essentially how I envision this working. I'll keep working on it after lunch).

Harbormaster completed remote builds in B970: Diff 22590.Jun 10 2014, 8:55 PM
epriestley added inline comments.Jun 10 2014, 9:09 PM
support/aphlict/client/src/AphlictMaster.as
150

Probably needs newPHIDs.length.

175

Likewise.

joshuaspence added inline comments.Jun 10 2014, 9:19 PM
support/aphlict/server/aphlict_server.js
102–104

I should try/catch this in case the data is not valid JSON.

173

Since the AphlictMaster needs to route notifications as well, perhaps the subscribers field should just be a part of msg.data?

webroot/rsrc/js/application/aphlict/Aphlict.js
54–57

Should this be moved into a this._flashContainer.subscribe call? Will a client ever need to change its subscriptions after being instantiated?

webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js
23

As above...

joshuaspence added inline comments.Jun 10 2014, 9:21 PM
support/aphlict/server/aphlict_server.js
173

At the same time... maybe not. If we just blindly pass this stuff on then it would be possible for the client to determine what other clients are listening to. Whether or not this is an issue, I am not sure.

epriestley added inline comments.Jun 10 2014, 9:29 PM
support/aphlict/server/aphlict_server.js
173

For now, I'd probably pass it as part of data.

In the future, we could send just the intersection of the subscriber lists to the master, maybe, to split the difference?

webroot/rsrc/js/application/aphlict/Aphlict.js
54–57

I don't have any plans which would require this.

joshuaspence updated this revision to Diff 22594.Jun 10 2014, 9:46 PM
  • Recover if passed non-JSON data
  • Pass subscribers to the Aphlict clients as well.
  • Fix function call
  • Handle bad data being POSTed to the Aphlict server.
  • Fix broken implementation
  • Further implementation
Harbormaster completed remote builds in B974: Diff 22594.Jun 10 2014, 9:47 PM
joshuaspence updated this revision to Diff 22596.Jun 10 2014, 9:48 PM
  • Explicitly check length
Harbormaster completed remote builds in B976: Diff 22596.Jun 10 2014, 9:49 PM
joshuaspence updated this revision to Diff 22597.Jun 10 2014, 9:50 PM
  • Include client ID in message
  • Add a log message
Harbormaster completed remote builds in B977: Diff 22597.Jun 10 2014, 9:51 PM
joshuaspence updated this revision to Diff 22610.Jun 10 2014, 10:16 PM
  • Explicitly set subscriptions
Harbormaster completed remote builds in B978: Diff 22610.Jun 10 2014, 10:17 PM
joshuaspence updated this revision to Diff 22617.Jun 10 2014, 11:09 PM
  • Send subscriptions on initial connection
joshuaspence added a comment.Jun 10 2014, 11:10 PM

There is some sort of race condition going on... Here is the output from the server:

[Tue Jun 10 2014 23:08:27 GMT+0000 (UTC)] <FlashPolicy> Policy Request From ::ffff:10.0.0.1
[Tue Jun 10 2014 23:08:27 GMT+0000 (UTC)] <Listener/6> Connected from ::ffff:10.0.0.1
[Tue Jun 10 2014 23:08:27 GMT+0000 (UTC)] <Listener/6> Received data: {"command":"subscribe","data":[]}
[Tue Jun 10 2014 23:08:27 GMT+0000 (UTC)] <Listener/6> Subscribed to: []
[Tue Jun 10 2014 23:08:29 GMT+0000 (UTC)] <Listener/6> Ended Connection
[Tue Jun 10 2014 23:08:29 GMT+0000 (UTC)] <Listener/6> Disconnected

At this point I refresh the page...

[Tue Jun 10 2014 23:08:30 GMT+0000 (UTC)] <FlashPolicy> Policy Request From ::ffff:10.0.0.1
[Tue Jun 10 2014 23:08:31 GMT+0000 (UTC)] <Listener/7> Connected from ::ffff:10.0.0.1
[Tue Jun 10 2014 23:08:31 GMT+0000 (UTC)] <Listener/7> Received data: {"data":["PHID-USER-cb5af6p4oepy5tlgqypi"],"command":"subscribe"}
[Tue Jun 10 2014 23:08:31 GMT+0000 (UTC)] <Listener/7> Subscribed to: ["PHID-USER-cb5af6p4oepy5tlgqypi"]
Harbormaster completed remote builds in B979: Diff 22617.Jun 10 2014, 11:10 PM
epriestley added a comment.Jun 10 2014, 11:39 PM

I caught a couple of things but I don't think they're the race. Let me patch this locally...

src/view/page/PhabricatorStandardPageView.php
389–391 ↗(On Diff #22617)

For logged-out users, this will add a null to the end of the list.

support/aphlict/client/src/AphlictMaster.as
62–64

These should probably be initialized before we connect to anything.

212

this.subscriptions[phid] might be undefined, so this could throw when accessing [client], I think.

joshuaspence updated this revision to Diff 22620.Jun 10 2014, 11:45 PM
  • Don't add null to $subscriptions for logged out view.
  • Reorder some stuff
  • this.subscriptions[phid] could be undefined.
Harbormaster completed remote builds in B981: Diff 22620.Jun 10 2014, 11:46 PM
epriestley added a comment.Jun 10 2014, 11:46 PM

Seems to be working correctly for me. I needed to make one small adjustment (buffer.readUInt16BE() requires a 0 in my version of Node) but can't repro the race issue after that tweak:

https://secure.phabricator.com/differential/diff/22621/

epriestley added a comment.Jun 10 2014, 11:53 PM

Maybe?

support/aphlict/client/src/AphlictMaster.as
132–137

We should probably check for phids.length before sending this. Also, all the "subscribe" stuff could go in a separate method.

157

this.socket may not exist yet or may not be connected yet. We should set some "connected" flag in didConnectSocket() and only send this if newPHIDs.length && this.isConnected.

182–189

Same as above.

joshuaspence updated this revision to Diff 22622.Jun 10 2014, 11:58 PM

Refactoring and rebased.

Harbormaster completed remote builds in B982: Diff 22622.Jun 10 2014, 11:59 PM
joshuaspence updated this revision to Diff 22704.Jun 11 2014, 7:09 PM

Fixed all the stuff that was broken.

Harbormaster completed remote builds in B1009: Diff 22704.Jun 11 2014, 7:10 PM
joshuaspence edited the test plan for this revision. (Show Details)Jun 11 2014, 7:12 PM
epriestley accepted this revision.Jun 11 2014, 7:16 PM
epriestley edited edge metadata.

On some operating systems, flush() is called automatically between execution frames, but on other operating systems, such as Windows, the data is never sent unless you call flush() explicitly. To ensure your application behaves reliably across all operating systems, it is a good practice to call the flush() method after writing each message (or related group of data) to the socket.

omg lul wut the huh how does thanks flash

This revision is now accepted and ready to land.Jun 11 2014, 7:16 PM
joshuaspence updated this object.Jun 11 2014, 7:17 PM
joshuaspence edited edge metadata.
epriestley added a comment.Jun 11 2014, 7:19 PM

Unrelated inline.

support/aphlict/server/aphlict_server.js
219

(Is this missing an argument?)

joshuaspence added inline comments.Jun 11 2014, 7:20 PM
support/aphlict/server/aphlict_server.js
219

Yep, it is... let me fix.

epriestley closed this revision.Jun 11 2014, 7:20 PM
epriestley updated this revision to Diff 22705.

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

Revision Contents
Changeset List

PathSize
src/
applications/
notification/
client/
2 lines
support/
aphlict/
client/
src/
10 lines
53 lines
server/
96 lines
lib/
4 lines
38 lines
24 lines
14 lines
2 lines
webroot/
rsrc/
js/
application/
aphlict/
9 lines
8 lines
swf/

Diff 22590

View Options

src/applications/notification/client/PhabricatorNotificationClient.php

Loading...
View Options

support/aphlict/client/src/AphlictClient.as

Loading...
View Options

support/aphlict/client/src/AphlictMaster.as

Loading...
View Options

support/aphlict/server/aphlict_server.js

Loading...
View Options

support/aphlict/server/lib/AphlictFlashPolicyServer.js

Loading...
View Options

support/aphlict/server/lib/AphlictListener.js

Loading...
View Options

support/aphlict/server/lib/AphlictListenerList.js

Loading...
View Options

support/aphlict/server/lib/AphlictLog.js

Loading...
View Options

support/aphlict/server/lib/javelin.js

Loading...
View Options

webroot/rsrc/js/application/aphlict/Aphlict.js

Loading...
View Options

webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js

Loading...
View Options

webroot/rsrc/swf/aphlict.swf

Loading...
Phabricator · Built by Phacility