Make the Aphlict server more resilient.
ClosedPublic
Actions

Authored by joshuaspence on Jun 11 2014, 5:07 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4324: Change notification server so it only alerts appropriate clients
T5284: Allow users to receive Aphlict notifications for an object they are viewing

Commits

Restricted Diffusion Commit
rPab4324148af7: Make the Aphlict server more resilient.

Summary

Currently, the Aphlict server will crash if invalid JSON data is POSTed to it. I have fixed this to, instead, return a 400. Also made some minor formatting changes.

Ref T4324. Ref T5284. Also, modify the data structure that is passed around (i.e. POSTed to the Aphlict server and broadcast to the Aphlict clients) to include the subscribers. Initially, I figured that we shouldn't expose this information to the clients... however, it is necessary for T4324 that the AphlictMaster is able to route a notification to the appropriate clients.

Test Plan

Making the following curl request: curl --data "{" http://localhost:22281/.

Before

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 17:07:51 GMT+0000 (UTC)] Started Server (PID 2033)
[Wed Jun 11 2014 17:07:55 GMT+0000 (UTC)] 
<<< UNCAUGHT EXCEPTION! >>>

SyntaxError: Unexpected end of input
>>> Server exited!

After
(No output... the bad JSON is caught and a 400 is returned)

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

joshuaspence updated this revision to Diff 22666.Jun 11 2014, 5:07 PM

joshuaspence retitled this revision from to Make the Aphlict server more resilient..

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 11 2014, 5:07 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B1004: Diff 22666.Jun 11 2014, 5:08 PM

One issue I think?

support/aphlict/server/aphlict_server.js
124	This is a functional change (msg.data -> msg) -- is that OK?

In D9480#6, @epriestley wrote:

One issue I think?

Sorry, I forgot that I made another change... let me update the description and test that notifications are still working.

joshuaspence updated this object.Jun 11 2014, 5:13 PM

joshuaspence added tasks: T5284: Allow users to receive Aphlict notifications for an object they are viewing, T4324: Change notification server so it only alerts appropriate clients.

epriestley added inline comments.Jun 11 2014, 5:15 PM

src/applications/feed/PhabricatorFeedStoryPublisher.php
178–182	Oh, durr, it should be OK because of this. This is probably fine. Anyway, double-check it and this looks good otherwise.

epriestley accepted this revision.Jun 11 2014, 5:16 PM

epriestley edited edge metadata.

This revision is now accepted and ready to land.Jun 11 2014, 5:16 PM

Yep, notifications still work (sent myself a few test notifications from /notification/status/)

Interestingly (and I've experienced this a few times), I have to close all of the https://secure.phabricator.com tabs that I had open in order to get this to work on my dev box. It's strange.