From a49855abfa5431711369cf3a94c0a213a0b92e92 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 13 Jul 2011 22:32:35 +0200 Subject: [PATCH] Improve error handling in connection code --- mopidy/utils/network.py | 28 ++++++++++++++++++++---- tests/utils/network_test.py | 43 ++++++++++++++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/mopidy/utils/network.py b/mopidy/utils/network.py index 52fc04eb..1d63f6e6 100644 --- a/mopidy/utils/network.py +++ b/mopidy/utils/network.py @@ -5,6 +5,7 @@ import re import socket import threading +from pykka import ActorDeadError from pykka.actor import ThreadingActor from pykka.registry import ActorRegistry @@ -138,7 +139,11 @@ class Connection(object): def stop(self, reason, level=logging.DEBUG): logger.log(level, reason) - self.actor_ref.stop() + try: + self.actor_ref.stop() + except ActorDeadError: + pass + self.disable_timeout() self.disable_recv() self.disable_send() @@ -169,10 +174,15 @@ class Connection(object): self.timeout_id = None def enable_recv(self): - if self.recv_id is None: + if self.recv_id is not None: + return + + try: self.recv_id = gobject.io_add_watch(self.sock.fileno(), gobject.IO_IN | gobject.IO_ERR | gobject.IO_HUP, self.recv_callback) + except socket.error as e: + self.stop(u'Problem with connection: %s' % e) def disable_recv(self): if self.recv_id is not None: @@ -180,10 +190,15 @@ class Connection(object): self.recv_id = None def enable_send(self): - if self.send_id is None: + if self.send_id is not None: + return + + try: self.send_id = gobject.io_add_watch(self.sock.fileno(), gobject.IO_OUT | gobject.IO_ERR | gobject.IO_HUP, self.send_callback) + except socket.error as e: + self.stop(u'Problem with connection: %s' % e) def disable_send(self): if self.send_id is not None: @@ -204,8 +219,13 @@ class Connection(object): if not data: self.stop(u'Client most likely disconnected.') - else: + return True + + try: self.actor_ref.send_one_way({'received': data}) + except ActorDeadError: + self.stop(u'Actor is dead.') + return True def send_callback(self, fd, flags): diff --git a/tests/utils/network_test.py b/tests/utils/network_test.py index e1f1bbed..9e41472f 100644 --- a/tests/utils/network_test.py +++ b/tests/utils/network_test.py @@ -1,12 +1,13 @@ import errno import gobject +import logging +import pykka import socket import unittest -import logging -from mock import patch, sentinel, Mock from mopidy.utils import network +from mock import patch, sentinel, Mock from tests import SkipTest, any_int, any_unicode class FormatHostnameTest(unittest.TestCase): @@ -59,6 +60,7 @@ class CreateSocketTest(unittest.TestCase): def test_ipv6_only_is_set(self): pass + class ServerTest(unittest.TestCase): def setUp(self): self.mock = Mock(spec=network.Server) @@ -205,6 +207,7 @@ class ServerTest(unittest.TestCase): (sentinel.host, sentinel.port)) sock.close.assert_called_once_with() + class ConnectionTest(unittest.TestCase): def setUp(self): self.mock = Mock(spec=network.Connection) @@ -272,6 +275,13 @@ class ConnectionTest(unittest.TestCase): network.Connection.stop(self.mock, sentinel.reason) self.mock.actor_ref.stop.assert_called_once_with() + def test_stop_handles_actor_already_being_stopped(self): + self.mock.actor_ref = Mock() + self.mock.actor_ref.stop.side_effect = pykka.ActorDeadError() + self.mock.sock = Mock(spec=socket.SocketType) + + network.Connection.stop(self.mock, sentinel.reason) + @patch.object(network.logger, 'log', new=Mock()) def test_stop_logs_reason(self): self.mock.actor_ref = Mock() @@ -327,6 +337,15 @@ class ConnectionTest(unittest.TestCase): self.assertEqual(0, gobject.source_remove.call_count) self.assertEqual(None, self.mock.recv_id) + def test_enable_recv_on_closed_socket(self): + self.mock.recv_id = None + self.mock.sock = Mock(spec=socket.SocketType) + self.mock.sock.fileno.side_effect = socket.error(errno.EBADF, '') + + network.Connection.enable_recv(self.mock) + self.mock.stop.assert_called_once_with(any_unicode) + self.assertEqual(None, self.mock.recv_id) + @patch.object(gobject, 'io_add_watch', new=Mock()) def test_enable_send_registers_with_gobject(self): self.mock.send_id = None @@ -363,6 +382,14 @@ class ConnectionTest(unittest.TestCase): self.assertEqual(0, gobject.source_remove.call_count) self.assertEqual(None, self.mock.send_id) + def test_enable_send_on_closed_socket(self): + self.mock.send_id = None + self.mock.sock = Mock(spec=socket.SocketType) + self.mock.sock.fileno.side_effect = socket.error(errno.EBADF, '') + + network.Connection.enable_send(self.mock) + self.assertEqual(None, self.mock.send_id) + @patch.object(gobject, 'timeout_add_seconds', new=Mock()) def test_enable_timeout_clears_existing_timeouts(self): self.mock.timeout = 10 @@ -453,7 +480,7 @@ class ConnectionTest(unittest.TestCase): sentinel.fd, gobject.IO_IN | gobject.IO_HUP | gobject.IO_ERR)) self.mock.stop.assert_called_once_with(any_unicode) - def test_recv_callback_gets_data(self): + def test_recv_callback_sends_data_to_actor(self): self.mock.sock = Mock(spec=socket.SocketType) self.mock.sock.recv.return_value = 'data' self.mock.actor_ref = Mock() @@ -463,6 +490,16 @@ class ConnectionTest(unittest.TestCase): self.mock.actor_ref.send_one_way.assert_called_once_with( {'received': 'data'}) + def test_recv_callback_handles_dead_actors(self): + self.mock.sock = Mock(spec=socket.SocketType) + self.mock.sock.recv.return_value = 'data' + self.mock.actor_ref = Mock() + self.mock.actor_ref.send_one_way.side_effect = pykka.ActorDeadError() + + self.assertTrue(network.Connection.recv_callback( + self.mock, sentinel.fd, gobject.IO_IN)) + self.mock.stop.assert_called_once_with(any_unicode) + def test_recv_callback_gets_no_data(self): self.mock.sock = Mock(spec=socket.SocketType) self.mock.sock.recv.return_value = ''