Ref T4324. Currently, Aphlict notifications are broadcast to all clients without any consideration as to whether the recipients will be interested in the notification. This differential allows clients to register their user PHID with the Aphlict server. Whilst the Aphlict server does not currently take advantage of the user PHIDs, this will theoretically allow the Aphlict server to broadcast notifications only to related clients.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4324: Change notification server so it only alerts appropriate clients
Ran the Aphlict server in debug mode:
> sudo ./bin/aphlict debug
Starting Aphlict server in foreground...
Launching server:
$ node '/usr/src/phabricator/src/applications/aphlict/management/../../../../support/aphlict/server/aphlict_server.js' --port='22280' --admin='22281' --host='localhost' --user='aphlict'
[Fri Jun 06 2014 16:57:57 GMT+0000 (UTC)] Started Server (PID 2044)
[Fri Jun 06 2014 16:57:59 GMT+0000 (UTC)] <FlashPolicy> Policy Request From ::ffff:10.0.0.1
[Fri Jun 06 2014 16:58:00 GMT+0000 (UTC)] <Listener/1> Connected from ::ffff:10.0.0.1
[Fri Jun 06 2014 16:58:00 GMT+0000 (UTC)] <Listener/1> Received data: {"data":"PHID-USER-cb5af6p4oepy5tlgqypi","command":"login"}
[Fri Jun 06 2014 16:58:00 GMT+0000 (UTC)] <Listener/1> Logged in as PHID-USER-cb5af6p4oepy5tlgqypiDiff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- aphlict-user
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 915 Build 915: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| support/aphlict/client/src/AphlictClient.as | ||
|---|---|---|
| 31–34 | I don't particularly like that these fields are duplicated in AphlictClient and AphlictMaster, but I don't have a better solution at the moment. | |
| support/aphlict/client/src/AphlictMaster.as | ||
| 125 | JSON is largely unnecessary here. Perhaps it is sufficient to simply send the user PHID over the socket (without any metadata such as "login")? | |
| support/aphlict/server/aphlict_server.js | ||
| 85–100 | I need to store this in a buffer in case it doesn't fit into one socket buffer. | |
Comment Actions
- Use ActionScript toJSON method.
- Only send notifications to subscribed users.
- Fix linter issues.
Comment Actions
- Use ActionScript toJSON method.
- Fix linter issues.
- Add a debug message on the Aphlict Server for login.
- Minor tidying.
Comment Actions
- Add a debug message on the Aphlict Server for login.
- Minor tidying.
- Improved log message.
- Minor tidying.
Comment Actions
Although a separate user is probably worth keeping, I think we should generalize this slightly to prepare for T5284.
Particularly, the pageObjects list passed a few lines below user has all of the important object PHIDs on the page, and user + pageObjects are all the things the client wants to listen to. Although we don't have a way to unlisten yet, and non-masters won't be able to extend the listen list, I think we end up with a shorter path forward if we send a listen ("listen to list of PHIDs") command now instead of a login ("login as user") command, and ideally get the pageObjects propagating in.
(In the long run, if we do add authentication, we'd probably put it as metadata on the command channel anyway, to reduce the number of round trips, so we would probably never re-introduce a "login" command, I'd guess?)
This is a little fuzzy and it's fairly easy to version things so I'm not too committed to it if it doesn't feel like a great approach, but basically I think:
- we should pretty-definitely use "listen" with a list of PHIDs as the command, instead of "login" with a single PHID;
- we should kinda-maybe pass pageObjects through now, too.
Notably, by merit of "every client gets every notification", listening to objects works properly (from a user perspective) today, so I'd prefer to tackle T5284 somewhat-soon-ish because losing it is a minor functional regression, even though it's a somewhat separate issue from T4324.
| support/aphlict/client/src/AphlictMaster.as | ||
|---|---|---|
| 127 | Let's make this listen and pass an array of PHIDs? | |
Comment Actions
Ah, I didn't realise that this works now... although, thinking about it, it makes sense.
Comment Actions
Actually, just to clarify.... this diff doesn't actually modify the delivery of notifications from server to clients. All notifications would still be routed to all clients. The server would just be made aware of the user PHID of all clients.
Comment Actions
Anyway, I think that there are a couple of minor reasons against using listen over login:
- Using login is more explicit in terms of meaning.
- Using listen means that a client could listen to multiple userPHIDs... which doesn't seem to make much sense (at least, not right now).
Comment Actions
Using listen means that a client could listen to multiple userPHIDs... which doesn't seem to make much sense (at least, not right now).
This doesn't make sense, but preventing it doesn't give us any benefits: someone who wants to snoop on metadata can connect to the server as many times as they want listening for one user per connection. Listening to a userPHID other than the logged-in user might be a meaningful use case in the future if you're looking at that user's profile page and we want to livestream their feed or something.
If we stuck with login, how do you imagine T5284 working?
Comment Actions
I guess I imagined that login and listen would be separate actions. I don't really mind either way, so I'll change this diff to use listen.