Details

Reviewers

Group 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

Lint

Lint Skipped

Unit

Tests Skipped

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

Implement the subscribe method.
Implement the unsubscribe method.

Harbormaster completed remote builds in B966: Diff 22579.Jun 10 2014, 7:24 PM

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?
87–88	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?
220–222	(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?
45–49	Consider `this._subscriptions[phid] = true;`
58–62	Consider `delete this._subscriptions[phid];`
68	Consider `isSubscribedToAny()` or similar, for clarity.
68–73	Consider `... if (phid in this._subscriptions) { return true; } ...`

This revision now requires changes to proceed.Jun 10 2014, 7:45 PM

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

Cool, this all seems pretty sensible to me.

support/aphlict/server/aphlict_server.js
92	Probably needs a `return;` here if length is less than 2.
93	Consider using 8-character padded ASCII text for consistency with transmissions and readability.
99–100	Probably needs `return;`.
support/aphlict/server/lib/AphlictListener.js
45–49	You could do this assignment/delete unconditionally if you want, they won't hurt anything.

Need to read more bytes
Buffer.concat is a static function
Process data in a loop
Be less conditional

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

Add client side support

(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
160	Probably needs `newPHIDs.length`.
185	Likewise.

joshuaspence added inline comments.Jun 10 2014, 9:19 PM

support/aphlict/server/aphlict_server.js
103–105	I should `try`/`catch` this in case the data is not valid JSON.
177–178	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–58	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
177–178	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
177–178	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–58	I don't have any plans which would require this.

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

Explicitly check length

Harbormaster completed remote builds in B976: Diff 22596.Jun 10 2014, 9:49 PM

Include client ID in message
Add a log message

Harbormaster completed remote builds in B977: Diff 22597.Jun 10 2014, 9:51 PM

Explicitly set subscriptions

Harbormaster completed remote builds in B978: Diff 22610.Jun 10 2014, 10:17 PM

Send subscriptions on initial connection

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

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
395–397	For logged-out users, this will add a `null` to the end of the list.
support/aphlict/client/src/AphlictMaster.as
64–65	These should probably be initialized before we connect to anything.
237	`this.subscriptions[phid]` might be `undefined`, so this could throw when accessing `[client]`, I think.

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

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/

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.
167	`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`.
192–199	Same as above.

Refactoring and rebased.

Harbormaster completed remote builds in B982: Diff 22622.Jun 10 2014, 11:59 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

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.

Unrelated inline.

support/aphlict/server/aphlict_server.js
234	(Is this missing an argument?)

joshuaspence added inline comments.Jun 11 2014, 7:20 PM

support/aphlict/server/aphlict_server.js
234	Yep, it is... let me fix.

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

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 22705

resources/celerity/map.php

src/view/page/PhabricatorStandardPageView.php

support/aphlict/client/src/AphlictClient.as

support/aphlict/client/src/AphlictMaster.as

support/aphlict/server/aphlict_server.js

support/aphlict/server/lib/AphlictListener.js

support/aphlict/server/lib/AphlictListenerList.js

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

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

webroot/rsrc/swf/aphlict.swf

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 22705

resources/celerity/map.php

src/view/page/PhabricatorStandardPageView.php

support/aphlict/client/src/AphlictClient.as

support/aphlict/client/src/AphlictMaster.as

support/aphlict/server/aphlict_server.js

support/aphlict/server/lib/AphlictListener.js

support/aphlict/server/lib/AphlictListenerList.js

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

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

webroot/rsrc/swf/aphlict.swf

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

Revision Contents
Changeset List