From: W. Trevor King Date: Fri, 7 Mar 2014 04:21:04 +0000 (-0800) Subject: irkerd: Pull request-parsing out into Irker._parse_request X-Git-Tag: 2.7~28 X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=8468af97bdd1e4ceaad9a68204565cfd808564fd;p=irker.git irkerd: Pull request-parsing out into Irker._parse_request There is a lot of error checking here, which is good, but it distracts from the core logic of Irker.handle. By pulling the parsing out into a private helper function, we isolate the code focused on parsing and error checking from the code focused on dispatching and connection management, making both easier to read. I've also changed the Target-validation logic. The old Target.valid returned True if the Target URL was valid, and False otherwise. The new Target.validate returns None, and raises an InvalidRequest exception with an error message describing exactly why the URL is invalid. We print these messages when dropping server URLs in Irker._parse_request, while the old Irker.handle code silently dropped invalid targets. We also continue processing other server URLs after an invalid Target, while the old Irker.handle code bailed out after the first invalid Target. Besides making the invalid URLs more obvious in the logs and increasing resiliency to invalid URLs, these changes allow us to pull the URL-to-Target conversion out of Irker.handle entirely, so it can focus more strongly on dispatch and connection management. --- diff --git a/irkerd b/irkerd index 70af6fa..ea5f25e 100755 --- a/irkerd +++ b/irkerd @@ -597,6 +597,7 @@ class Connection: class Target(): "Represent a transmission target." def __init__(self, url): + self.url = url # Pre-2.6 Pythons don't recognize irc: as a valid URL prefix. url = url.replace("irc://", "http://") parsed = urlparse.urlparse(url) @@ -623,9 +624,14 @@ class Target(): if parsed.query: self.key = re.sub("^key=", "", parsed.query) self.port = int(ircport) - def valid(self): - "Both components must be present for a valid target." - return self.servername and self.channel + def validate(self): + "Raise InvalidRequest if the URL is missing a critical component" + if not self.servername: + raise InvalidRequest( + 'target URL missing a servername: %r' % self.url) + if not self.channel: + raise InvalidRequest( + 'target URL missing a channel: %r' % self.url) def server(self): "Return a hashable tuple representing the destination server." return (self.servername, self.port) @@ -763,69 +769,72 @@ class Irker: def pending(self): "Do we have any pending message traffic?" return [k for (k, v) in self.servers.items() if v.pending()] + + def _parse_request(self, line): + "Request-parsing helper for the handle() method" + request = json.loads(line.strip()) + if not isinstance(request, dict): + raise InvalidRequest( + "request is not a JSON dictionary: %r" % request) + if "to" not in request or "privmsg" not in request: + raise InvalidRequest( + "malformed request - 'to' or 'privmsg' missing: %r" % request) + channels = request['to'] + message = request['privmsg'] + if not isinstance(channels, (list, basestring)): + raise InvalidRequest( + "malformed request - unexpected channel type: %r" % channels) + if not isinstance(message, basestring): + raise InvalidRequest( + "malformed request - unexpected message type: %r" % message) + if not isinstance(channels, list): + channels = [channels] + targets = [] + for url in channels: + try: + if not isinstance(url, basestring): + raise InvalidRequest( + "malformed request - URL has unexpected type: %r" % + url) + target = Target(url) + target.validate() + except InvalidRequest, e: + self.logerr(str(e)) + else: + targets.append(target) + return (targets, message) + def handle(self, line, quit_after=False): "Perform a JSON relay request." try: - request = json.loads(line.strip()) - if not isinstance(request, dict): - raise InvalidRequest( - "request is not a JSON dictionary: %r" % request) - if "to" not in request or "privmsg" not in request: - raise InvalidRequest( - "malformed request - 'to' or 'privmsg' missing: %r" % - request) - channels = request['to'] - message = request['privmsg'] - if not isinstance(channels, (list, basestring)): - raise InvalidRequest( - "malformed request - unexpected channel type: %r" - % channels) - if not isinstance(message, basestring): - raise InvalidRequest( - "malformed request - unexpected message type: %r" % - message) - if not isinstance(channels, list): - channels = [channels] - for url in channels: - try: - if not isinstance(url, basestring): - raise InvalidRequest( - "malformed request - URL has unexpected type: %r" % - url) - except InvalidRequest, e: - self.logerr(str(e)) - else: - target = Target(url) - if not target.valid(): - return - if target.server() not in self.servers: - self.servers[target.server()] = Dispatcher( - self, target.servername, target.port) - self.servers[target.server()].dispatch( - target.channel, message, target.key, - quit_after=quit_after) - # GC dispatchers with no active connections - servernames = self.servers.keys() - for servername in servernames: - if not self.servers[servername].live(): - del self.servers[servername] - # If we might be pushing a resource limit even - # after garbage collection, remove a session. - # The goal here is to head off DoS attacks - # that aim at exhausting thread space or file - # descriptors. The cost is that attempts to - # DoS this service will cause lots of - # join/leave spam as we scavenge old channels - # after connecting to new ones. The particular - # method used for selecting a session to be - # terminated doesn't matter much; we choose - # the one longest idle on the assumption that - # message activity is likely to be clumpy. - if len(self.servers) >= CONNECTION_MAX: - oldest = min( - self.servers.keys(), - key=lambda name: self.servers[name].last_xmit()) - del self.servers[oldest] + targets, message = self._parse_request(line=line) + for target in targets: + if target.server() not in self.servers: + self.servers[target.server()] = Dispatcher( + self, target.servername, target.port) + self.servers[target.server()].dispatch( + target.channel, message, target.key, quit_after=quit_after) + # GC dispatchers with no active connections + servernames = self.servers.keys() + for servername in servernames: + if not self.servers[servername].live(): + del self.servers[servername] + # If we might be pushing a resource limit even + # after garbage collection, remove a session. The + # goal here is to head off DoS attacks that aim at + # exhausting thread space or file descriptors. + # The cost is that attempts to DoS this service + # will cause lots of join/leave spam as we + # scavenge old channels after connecting to new + # ones. The particular method used for selecting a + # session to be terminated doesn't matter much; we + # choose the one longest idle on the assumption + # that message activity is likely to be clumpy. + if len(self.servers) >= CONNECTION_MAX: + oldest = min( + self.servers.keys(), + key=lambda name: self.servers[name].last_xmit()) + del self.servers[oldest] except InvalidRequest, e: self.logerr(str(e)) except ValueError: