Skip to content

Commit ab43241

Browse files
joshuaspenceepriestley
authored andcommitted
Make the Aphlict server more resilient.
Summary: Currently, the Aphlict server will crash if invalid JSON data is `POST`ed 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. `POST`ed 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) Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley, Korvin Maniphest Tasks: T4324, T5284 Differential Revision: https://secure.phabricator.com/D9480
1 parent bb06e36 commit ab43241

File tree

10 files changed

+66
-63
lines changed

10 files changed

+66
-63
lines changed

resources/celerity/map.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@
476476
'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8',
477477
'rsrc/js/phuix/PHUIXActionView.js' => '6e8cefa4',
478478
'rsrc/js/phuix/PHUIXDropdownMenu.js' => 'bd4c8dca',
479-
'rsrc/swf/aphlict.swf' => 'b7c2d7aa',
479+
'rsrc/swf/aphlict.swf' => 'f45c3edc',
480480
),
481481
'symbols' =>
482482
array(

src/applications/feed/PhabricatorFeedStoryPublisher.php

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -176,10 +176,8 @@ private function insertNotifications($chrono_key) {
176176

177177
private function sendNotification($chrono_key) {
178178
$data = array(
179-
'data' => array(
180-
'key' => (string)$chrono_key,
181-
'type' => 'notification',
182-
),
179+
'key' => (string)$chrono_key,
180+
'type' => 'notification',
183181
'subscribers' => $this->subscribedPHIDs,
184182
);
185183

src/applications/notification/client/PhabricatorNotificationClient.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
final class PhabricatorNotificationClient {
44

5-
const EXPECT_VERSION = 5;
5+
const EXPECT_VERSION = 6;
66

77
public static function getServerStatus() {
88
$uri = PhabricatorEnv::getEnvConfig('notification.server-uri');

support/aphlict/server/aphlict_server.js

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,21 @@ if (config.logfile) {
2525

2626
function parse_command_line_arguments(argv) {
2727
var config = {
28-
port : 22280,
29-
admin : 22281,
30-
host : '127.0.0.1',
31-
user : null,
28+
port: 22280,
29+
admin: 22281,
30+
host: '127.0.0.1',
31+
user: null,
3232
log: '/var/log/aphlict.log'
3333
};
3434

3535
for (var ii = 2; ii < argv.length; ii++) {
3636
var arg = argv[ii];
3737
var matches = arg.match(/^--([^=]+)=(.*)$/);
3838
if (!matches) {
39-
throw new Error("Unknown argument '"+arg+"'!");
39+
throw new Error("Unknown argument '" + arg + "'!");
4040
}
4141
if (!(matches[1] in config)) {
42-
throw new Error("Unknown argument '"+matches[1]+"'!");
42+
throw new Error("Unknown argument '" + matches[1] + "'!");
4343
}
4444
config[matches[1]] = matches[2];
4545
}
@@ -52,19 +52,19 @@ function parse_command_line_arguments(argv) {
5252

5353
if (process.getuid() !== 0) {
5454
console.log(
55-
"ERROR: "+
56-
"This server must be run as root because it needs to bind to privileged "+
57-
"port 843 to start a Flash policy server. It will downgrade to run as a "+
58-
"less-privileged user after binding if you pass a user in the command "+
55+
"ERROR: " +
56+
"This server must be run as root because it needs to bind to privileged " +
57+
"port 843 to start a Flash policy server. It will downgrade to run as a " +
58+
"less-privileged user after binding if you pass a user in the command " +
5959
"line arguments with '--user=alincoln'.");
6060
process.exit(1);
6161
}
6262

6363
var net = require('net');
64-
var http = require('http');
64+
var http = require('http');
6565
var url = require('url');
6666

67-
process.on('uncaughtException', function (err) {
67+
process.on('uncaughtException', function(err) {
6868
debug.log("\n<<< UNCAUGHT EXCEPTION! >>>\n\n" + err);
6969
process.exit(1);
7070
});
@@ -95,7 +95,7 @@ var send_server = net.createServer(function(socket) {
9595
debug.log('<%s> Ended Connection', listener.getDescription());
9696
});
9797

98-
socket.on('error', function (e) {
98+
socket.on('error', function(e) {
9999
debug.log('<%s> Error: %s', listener.getDescription(), e);
100100
});
101101

@@ -107,23 +107,29 @@ var messages_in = 0;
107107
var start_time = new Date().getTime();
108108

109109
var receive_server = http.createServer(function(request, response) {
110-
response.writeHead(200, {'Content-Type' : 'text/plain'});
111-
112110
// Publishing a notification.
113111
if (request.method == 'POST') {
114112
var body = '';
115113

116-
request.on('data', function (data) {
114+
request.on('data', function(data) {
117115
body += data;
118116
});
119117

120-
request.on('end', function () {
121-
++messages_in;
122-
123-
var msg = JSON.parse(body);
124-
debug.log('notification: ' + JSON.stringify(msg));
125-
broadcast(msg.data);
126-
response.end();
118+
request.on('end', function() {
119+
try {
120+
var msg = JSON.parse(body);
121+
122+
debug.log('notification: ' + JSON.stringify(msg));
123+
++messages_in;
124+
broadcast(msg);
125+
126+
response.writeHead(200, {'Content-Type': 'text/plain'});
127+
} catch (err) {
128+
response.statusCode = 400;
129+
response.write('400 Bad Request');
130+
} finally {
131+
response.end();
132+
}
127133
});
128134
} else if (request.url == '/status/') {
129135
request.on('data', function(data) {
@@ -139,9 +145,10 @@ var receive_server = http.createServer(function(request, response) {
139145
'messages.in': messages_in,
140146
'messages.out': messages_out,
141147
'log': config.log,
142-
'version': 5
148+
'version': 6
143149
};
144150

151+
response.writeHead(200, {'Content-Type': 'text/plain'});
145152
response.write(JSON.stringify(status));
146153
response.end();
147154
});

support/aphlict/server/lib/AphlictFlashPolicyServer.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,12 @@ JX.install('AphlictFlashPolicyServer', {
1717
_accessPort: null,
1818
_debug: null,
1919

20-
setDebugLog : function(log) {
20+
setDebugLog: function(log) {
2121
this._debug = log;
2222
return this;
2323
},
2424

25-
setAccessPort : function(port) {
25+
setAccessPort: function(port) {
2626
this._accessPort = port;
2727
return this;
2828
},

support/aphlict/server/lib/AphlictListener.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,28 @@
11
var JX = require('javelin').JX;
22

33
JX.install('AphlictListener', {
4-
construct : function(id, socket) {
4+
construct: function(id, socket) {
55
this._id = id;
66
this._socket = socket;
77
},
88

9-
members : {
10-
_id : null,
11-
_socket : null,
9+
members: {
10+
_id: null,
11+
_socket: null,
1212

13-
getID : function() {
13+
getID: function() {
1414
return this._id;
1515
},
1616

17-
getSocket : function() {
17+
getSocket: function() {
1818
return this._socket;
1919
},
2020

21-
getDescription : function() {
21+
getDescription: function() {
2222
return 'Listener/' + this.getID();
2323
},
2424

25-
writeMessage : function(message) {
25+
writeMessage: function(message) {
2626
var serial = JSON.stringify(message);
2727

2828
var length = Buffer.byteLength(serial, 'utf8');

support/aphlict/server/lib/AphlictListenerList.js

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,18 @@ var JX = require('javelin').JX;
22
JX.require('AphlictListener', __dirname);
33

44
JX.install('AphlictListenerList', {
5-
construct : function() {
5+
construct: function() {
66
this._listeners = {};
77
},
88

9-
members : {
10-
_listeners : null,
11-
_nextID : 0,
12-
_activeListenerCount : 0,
13-
_totalListenerCount : 0,
9+
members: {
10+
_listeners: null,
11+
_nextID: 0,
12+
_activeListenerCount: 0,
13+
_totalListenerCount: 0,
1414

15-
addListener : function(socket) {
16-
var listener = new JX.AphlictListener(
17-
this._generateNextID(),
18-
socket);
15+
addListener: function(socket) {
16+
var listener = new JX.AphlictListener(this._generateNextID(), socket);
1917

2018
this._listeners[listener.getID()] = listener;
2119
this._activeListenerCount++;
@@ -24,27 +22,27 @@ JX.install('AphlictListenerList', {
2422
return listener;
2523
},
2624

27-
removeListener : function(listener) {
25+
removeListener: function(listener) {
2826
var id = listener.getID();
2927
if (id in this._listeners) {
3028
delete this._listeners[id];
3129
this._activeListenerCount--;
3230
}
3331
},
3432

35-
getListeners : function() {
33+
getListeners: function() {
3634
return this._listeners;
3735
},
3836

39-
getActiveListenerCount : function() {
37+
getActiveListenerCount: function() {
4038
return this._activeListenerCount;
4139
},
4240

43-
getTotalListenerCount : function() {
41+
getTotalListenerCount: function() {
4442
return this._totalListenerCount;
4543
},
4644

47-
_generateNextID : function() {
45+
_generateNextID: function() {
4846
do {
4947
this._nextID = ((this._nextID + 1) % 1000000000000);
5048
} while (this._nextID in this._listeners);

support/aphlict/server/lib/AphlictLog.js

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,16 @@ var fs = require('fs');
44
var util = require('util');
55

66
JX.install('AphlictLog', {
7-
construct : function() {
7+
construct: function() {
88
this._writeToLogs = [];
99
this._writeToConsoles = [];
1010
},
1111

12-
members : {
13-
_writeToConsoles : null,
14-
_writeToLogs : null,
12+
members: {
13+
_writeToConsoles: null,
14+
_writeToLogs: null,
1515

16-
addLogfile : function(path) {
16+
addLogfile: function(path) {
1717
var options = {
1818
flags: 'a',
1919
encoding: 'utf8',
@@ -27,12 +27,12 @@ JX.install('AphlictLog', {
2727
return this;
2828
},
2929

30-
addConsole : function(console) {
30+
addConsole: function(console) {
3131
this._writeToConsoles.push(console);
3232
return this;
3333
},
3434

35-
log : function(pattern) {
35+
log: function(pattern) {
3636
var str = util.format.apply(null, arguments);
3737
var date = new Date().toLocaleString();
3838
str = '[' + date + '] ' + str;

support/aphlict/server/lib/javelin.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ JX.require('core/install');
66

77
// NOTE: This is faking out a piece of code in JX.install which waits for
88
// Stratcom before running static initializers.
9-
JX.Stratcom = {ready : true};
9+
JX.Stratcom = {ready: true};
1010
JX.require('core/Event');
1111
JX.require('core/Stratcom');
1212

webroot/rsrc/swf/aphlict.swf

3 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)