Gather more OSError exception in musicpd.ConnectionError

diff --git a/CHANGES.txt b/CHANGES.txt
index d32426aa682bdcdd48a44e1d9d909983dcead397..52e46d1d98861001f11c65b9f498f6726dc48992 100644 (file)
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -10,6 +10,8 @@ Changes in 0.9.0
  * Improved Range object to deal with window parameter
  * Add logging
  * Switch to pyproject.toml (setuptools build system)
+ * The connect sequence raises ConnectionError only on error,
+   previously getaddrinfo or unix socket connection error raised an OSError
 
 Changes in 0.8.0
 ----------------

diff --git a/musicpd.py b/musicpd.py
index 8b5d789ad86d291c85633415e3f3547d582d1fab..d8dc01b9048510a5b93ae00f652c5471e2674259 100644 (file)
--- a/musicpd.py
+++ b/musicpd.py
@@ -19,7 +19,7 @@ ERROR_PREFIX = "ACK "
 SUCCESS = "OK"
 NEXT = "list_OK"
 #: Module version
-VERSION = '0.9.0b1'
+VERSION = '0.9.0b2'
 #: Seconds before a connection attempt times out
 #: (overriden by :envvar:`MPD_TIMEOUT` env. var.)
 CONNECTION_TIMEOUT = 30
@@ -642,10 +642,13 @@ class MPDClient:
         # abstract socket
         if path.startswith('@'):
             path = '\0'+path[1:]
-        sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
-        sock.settimeout(self.mpd_timeout)
-        sock.connect(path)
-        sock.settimeout(self.socket_timeout)
+        try:
+            sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+            sock.settimeout(self.mpd_timeout)
+            sock.connect(path)
+            sock.settimeout(self.socket_timeout)
+        except socket.error as socket_err:
+            raise ConnectionError(socket_err.strerror) from socket_err
         return sock
 
     def _connect_tcp(self, host, port):
@@ -654,23 +657,29 @@ class MPDClient:
         except AttributeError:
             flags = 0
         err = None
-        for res in socket.getaddrinfo(host, port, socket.AF_UNSPEC,
-                                      socket.SOCK_STREAM, socket.IPPROTO_TCP,
-                                      flags):
+        try:
+            gai = socket.getaddrinfo(host, port, socket.AF_UNSPEC,
+                                     socket.SOCK_STREAM, socket.IPPROTO_TCP,
+                                     flags)
+        except socket.error as gaierr:
+            raise ConnectionError(gaierr.strerror) from gaierr
+        for res in gai:
             af, socktype, proto, _, sa = res
             sock = None
             try:
+                log.debug('opening socket %s', sa)
                 sock = socket.socket(af, socktype, proto)
                 sock.settimeout(self.mpd_timeout)
                 sock.connect(sa)
                 sock.settimeout(self.socket_timeout)
                 return sock
             except socket.error as socket_err:
+                log.debug('opening socket %s failed: %s}', sa, socket_err)
                 err = socket_err
                 if sock is not None:
                     sock.close()
         if err is not None:
-            raise ConnectionError(str(err))
+            raise ConnectionError(err.strerror)
         raise ConnectionError("getaddrinfo returns an empty list")
 
     def noidle(self):

diff --git a/test.py b/test.py
index 033fe061d54b3fa793315a5ba658e0538ffa5106..a123b306fcdfd58df0d06e6370bf192c9c8bcce6 100755 (executable)
--- a/test.py
+++ b/test.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 # coding: utf-8
-# SPDX-FileCopyrightText: 2012-2023  kaliko <kaliko@azylum.org>
+# SPDX-FileCopyrightText: 2012-2024  kaliko <kaliko@azylum.org>
 # SPDX-License-Identifier: LGPL-3.0-or-later
 # pylint: disable=missing-docstring
 """
@@ -14,11 +14,12 @@ import itertools
 import os
 import types
 import unittest
-import unittest.mock as mock
+import unittest.mock
 import warnings
 
 import musicpd
 
+mock = unittest.mock
 
 # show deprecation warnings
 warnings.simplefilter('default')
@@ -27,7 +28,7 @@ warnings.simplefilter('default')
 TEST_MPD_HOST, TEST_MPD_PORT = ('example.com', 10000)
 
 
-class testEnvVar(unittest.TestCase):
+class TestEnvVar(unittest.TestCase):
 
     def test_envvar(self):
         # mock "os.path.exists" here to ensure there are no socket in
@@ -559,7 +560,7 @@ class TestMPDClient(unittest.TestCase):
         with self.assertRaises(AttributeError):
             self.client.foo_bar()
 
-class testConnection(unittest.TestCase):
+class TestConnection(unittest.TestCase):
 
     def test_exposing_fileno(self):
         with mock.patch('musicpd.socket') as socket_mock:
@@ -613,14 +614,72 @@ class testConnection(unittest.TestCase):
             # Set socket timeout Raises Exception
             with self.assertRaises(ValueError):
                 cli.socket_timeout = 'foo'
-            with self.assertRaises(ValueError, msg='socket_timeout expects a non zero positive integer'):
+            with self.assertRaises(ValueError,
+                    msg='socket_timeout expects a non zero positive integer'):
                 cli.socket_timeout = '0'
-            with self.assertRaises(ValueError, msg='socket_timeout expects a non zero positive integer'):
+            with self.assertRaises(ValueError,
+                    msg='socket_timeout expects a non zero positive integer'):
                 cli.socket_timeout = '-1'
 
-class testException(unittest.TestCase):
 
-    def test_CommandError_on_newline(self):
+class TestConnectionError(unittest.TestCase):
+
+    @mock.patch('socket.socket')
+    def test_connect_unix(self, socket_mock):
+        """Unix socket socket.error should raise a musicpd.ConnectionError"""
+        mocked_socket = socket_mock.return_value
+        mocked_socket.connect.side_effect = musicpd.socket.error(42, 'err 42')
+        os.environ['MPD_HOST'] = '/run/mpd/socket'
+        cli = musicpd.MPDClient()
+        with self.assertRaises(musicpd.ConnectionError) as cme:
+            cli.connect()
+        self.assertEqual('err 42', str(cme.exception))
+
+    def test_non_available_unix_socket(self):
+        delattr(musicpd.socket, 'AF_UNIX')
+        os.environ['MPD_HOST'] = '/run/mpd/socket'
+        cli = musicpd.MPDClient()
+        with self.assertRaises(musicpd.ConnectionError) as cme:
+            cli.connect()
+        self.assertEqual('Unix domain sockets not supported on this platform',
+                         str(cme.exception))
+
+    @mock.patch('socket.getaddrinfo')
+    def test_connect_tcp_getaddrinfo(self, gai_mock):
+        """TCP socket.gaierror should raise a musicpd.ConnectionError"""
+        gai_mock.side_effect = musicpd.socket.error(42, 'gaierr 42')
+        cli = musicpd.MPDClient()
+        with self.assertRaises(musicpd.ConnectionError) as cme:
+            cli.connect(host=TEST_MPD_HOST)
+        self.assertEqual('gaierr 42', str(cme.exception))
+
+    @mock.patch('socket.getaddrinfo')
+    @mock.patch('socket.socket')
+    def test_connect_tcp_connect(self, socket_mock, gai_mock):
+        """A socket.error should raise a musicpd.ConnectionError
+        Mocking getaddrinfo to prevent network access (DNS)
+        """
+        gai_mock.return_value = [range(5)]
+        mocked_socket = socket_mock.return_value
+        mocked_socket.connect.side_effect = musicpd.socket.error(42, 'tcp conn err 42')
+        cli = musicpd.MPDClient()
+        with self.assertRaises(musicpd.ConnectionError) as cme:
+            cli.connect(host=TEST_MPD_HOST)
+        self.assertEqual('tcp conn err 42', str(cme.exception))
+
+    @mock.patch('socket.getaddrinfo')
+    def test_connect_tcp_connect_empty_gai(self, gai_mock):
+        """An empty getaddrinfo should raise a musicpd.ConnectionError"""
+        gai_mock.return_value = []
+        cli = musicpd.MPDClient()
+        with self.assertRaises(musicpd.ConnectionError) as cme:
+            cli.connect(host=TEST_MPD_HOST)
+        self.assertEqual('getaddrinfo returns an empty list', str(cme.exception))
+
+
+class TestCommandErrorException(unittest.TestCase):
+
+    def test_error_on_newline(self):
         os.environ['MPD_HOST'] = '/run/mpd/socket'
         with mock.patch('musicpd.socket') as socket_mock:
             sock = mock.MagicMock(name='socket')