[tor-dev] Erebus Code Review
Damian Johnson
atagar at torproject.org
Sat Sep 5 20:02:29 UTC 2015
Hi Cristobal, just took a peek at Erebus and looks good! There's some
low hanging fruit for improvement (unused functions and such) but
generally looks good.
Cheers! -Damian
======================================================================
erebus/server/handlers/log.py
======================================================================
9 import erebus.util.log
Perfectly fine for now, but obviously most of this is copy/paste from
Nyx. Both Nyx and Erebus are smack in the middle of development so
this is great! Do whatever you find most expedient. But obviously in
the long run we'll want to revisit this.
----------------------------------------------------------------------
16 def conf_handler(key, value):
17 if key == 'features.log.maxLinesPerEntry':
18 return max(1, value)
19 elif key == 'features.log.prepopulateReadLimit':
20 return max(0, value)
21 elif key == 'features.log.maxRefreshRate':
22 return max(10, value)
23 elif key == 'cache.log_panel.size':
24 return max(1000, value)
I know this is copy-and-paste from Nyx but personally I wouldn't
bother. Many of the config keys are unused, and at present users
don't have a way of customizing them. Nyx's support for a nyxrc is
a very rarely used feature, so I would leave this out for now.
----------------------------------------------------------------------
40 TOR_EVENT_TYPES = {
41 'd': 'DEBUG',
42 'i': 'INFO',
43 'n': 'NOTICE',
44 'w': 'WARN',
45 'e': 'ERR',
I did this for Nyx because because... well, curses. For Erebus we
can do better. ;)
Rather than 'key => event' mapping maybe Erebus should show all the
event types, with checkboxes for selection?
----------------------------------------------------------------------
155 def _register_event(self, event):
156 output = { 'message': '', 'type': ''}
157 if event.type not in self._logged_events:
158 return
159 #self._event_log.add(event)
160 #self._log_file.write(event.display_message)
161
162 # notifies the display that it has new content
163 if self._filter.match(event.display_message):
164 self._has_new_event = True
165 output['message'] = event.display_message
166 output['type'] = event.type.lower()
167 ws = ws_controller()
168 if ws is not None:
169 ws.send_data(WebSocketType.LOG, output)
You can drop the line with self._has_new_event. It's an unused
attribute, and doesn't appear outside of this line.
The 'output' is only ever used on this last line, so might as well
just call...
ws.send_data(WebSocketType.LOG, {'message': event.display_message,
'type': event.type.lower()})
----------------------------------------------------------------------
171 def log_handler():
172 """
173 Singleton for getting our log hander.
174
175 :returns: :class:`~erebus.server.handlers.log.LogHandler`
176 """
177
178 return LOG_HANDLER
179
180 def init_log_handler(logged_events):
181 """
182 """
183 global LOG_HANDLER
184 LOG_HANDLER = LogHandler(logged_events)
185 return LOG_HANDLER
Seems from a quick look that these might be unused. The controller.py
and starter.py call init_log_handler, but not spotting anything that
ever uses LOG_HANDLER.
----------------------------------------------------------------------
195 response = None
196 if controller is not None:
197 response = controller.get_info('events/names', None)
198 if response is None:
199 return [] # GETINFO query failed
This is equivalent to...
response = controller.get_info('events/names', []) if controller else []
======================================================================
erebus/util/controller.py
======================================================================
29 def erebus_controller():
30 """
31 Provides the erebus controller instance.
32 """
33
34 return EREBUS_CONTROLLER
35
36 def init_controller(control_port, control_socket, logged_events):
37 """
38 Spawns the controller
39 """
40
41 global EREBUS_CONTROLLER
42 EREBUS_CONTROLLER = Controller(control_port, control_socket,
logged_events)
Again, you call init_controller() but looks as though
EREBUS_CONTROLLER is never used.
----------------------------------------------------------------------
61 self._logged_events = logged_events
62
63 self._bw_handler = None
64 self._status_handler = None
65 self._log_handler = None
All unused.
----------------------------------------------------------------------
86 def _stop_loop_check(self):
87 """
88 Stop the looping task when is not necessary
89 """
90
91 self._loop_call.stop()
Only called once, in _conn_starter(). Might as well just call this
there (it's just a minor alias).
----------------------------------------------------------------------
156 def heartbeat_check(is_unresponsive):
157 """
158 Logs if its been ten seconds since the last BW event.
159
160 Arguments:
161 is_unresponsive - flag for if we've indicated to be
responsive or not
162 """
163
164 controller = tor_controller()
165 last_heartbeat = controller.get_latest_heartbeat()
166
167 if controller.is_alive():
168 if not is_unresponsive and (time.time() - last_heartbeat) >= 10:
169 is_unresponsive = True
170 log.notice('Relay unresponsive (last heartbeat:
%s)' % time.ctime(last_heartbeat))
171 elif is_unresponsive and (time.time() - last_heartbeat) < 10:
172 # really shouldn't happen (meant Tor froze for a bit)
173 is_unresponsive = False
174 log.notice('Relay resumed')
175
176 return is_unresponsive
Unused.
======================================================================
erebus/util/websockets.py
======================================================================
41 def add_websocket(self, ws_type, ws):
42 """
43 Add an object to the list of active websockets of certain type.
44
45 Arguments:
46 ws_type - type of websocket to be fetched
47 ws - websocket object to be added
48 """
49
50 if self._websockets[ws_type] is not None:
51 if ws not in self._websockets[ws_type]:
52 self._websockets[ws_type].append(ws)
53 else:
54 log.notice('Undefined type (%s) when adding websocket'
% ws_type)
Your 'if self._websockets[ws_type] is not None' check will never
evaluate to False. You default all WebSocketType values to an empty
list, and if it doesn't belong to that enum then it'll raise a
KeyError.
You want...
if ws_type in self._websockets:
Also, if self._websockets had sets rather than lists then you could
drop the second check.
Btw, lets use Sphinx pydocs. The documentation style you're using
here is adopted from Nyx before I started using Sphinx, and I'm
replacing that documentation style as I rewrite it.
More information about the tor-dev
mailing list