Fix Controller Bug on hostname="" or IPv6 not available (PR #248, Issue

#244)

* Implement _factory_invoked Event
* Suppress only socket.timeout exception
  Other exceptions should bubble up, no longer hidden
* Enforce ready_timeout
* Floatify AIOSMTPD_CONTROLLER_TIMEOUT value
* Update docs & NEWS
* Version bump
* Add a smarter* way to determine localhost addr
* Add test for hostname not specified

.github/workflows/unit-testing-and-coverage.yml

@@ -70,7 +70,7 @@ jobs:
       # If a matrix fail, do NOT stop other matrix, let them run to completion
       fail-fast: false
       matrix:
-        os: [ "ubuntu-18.04", "ubuntu-20.04", "macos-10.15", "windows-latest" ]
+        os: [ "macos-10.15", "ubuntu-18.04", "ubuntu-20.04", "windows-latest" ]
         python-version: [ "3.6", "3.7", "3.8", "3.9", "pypy3" ]
     runs-on: ${{ matrix.os }}
     steps:

aiosmtpd/__init__.py

@@ -1,4 +1,4 @@
 # Copyright 2014-2021 The aiosmtpd Developers
 # SPDX-License-Identifier: Apache-2.0
 
-__version__ = "1.3.0"
+__version__ = "1.3.1"

aiosmtpd/controller.py

@@ -2,11 +2,15 @@
 # SPDX-License-Identifier: Apache-2.0
 
 import asyncio
+import errno
 import os
 import ssl
 import threading
+import time
 from contextlib import ExitStack
-from socket import create_connection
+from socket import AF_INET6, SOCK_STREAM, create_connection, has_ipv6
+from socket import socket as makesock
+from socket import timeout as socket_timeout
 from typing import Any, Coroutine, Dict, Optional
 from warnings import warn
 
@@ -17,6 +21,42 @@
 AsyncServer = asyncio.base_events.Server
 
 
+def _has_ipv6():
+    # Helper function to assist in mocking
+    return has_ipv6
+
+
+def get_localhost() -> str:
+    # Ref:
+    #  - https://github.com/urllib3/urllib3/pull/611#issuecomment-100954017
+    #  - https://github.com/python/cpython/blob/ :
+    #    - v3.6.13/Lib/test/support/__init__.py#L745-L758
+    #    - v3.9.1/Lib/test/support/socket_helper.py#L124-L137
+    if not _has_ipv6():
+        # socket.has_ipv6 only tells us of current Python's IPv6 support, not the
+        # system's. But if the current Python does not support IPv6, it's pointless to
+        # explore further.
+        return "127.0.0.1"
+    try:
+        with makesock(AF_INET6, SOCK_STREAM) as sock:
+            sock.bind(("::1", 0))
+        # If we reach this point, that means we can successfully bind ::1 (on random
+        # unused port), so IPv6 is definitely supported
+        return "::1"
+    except OSError as e:
+        # Apparently errno.E* constants adapts to the OS, so on Windows they will
+        # automatically use the WSAE* constants
+        if e.errno == errno.EADDRNOTAVAIL:
+            # Getting (WSA)EADDRNOTAVAIL means IPv6 is not supported
+            return "127.0.0.1"
+        if e.errno == errno.EADDRINUSE:
+            # Getting (WSA)EADDRINUSE means IPv6 *is* supported, but already used.
+            # Shouldn't be possible, but just in case...
+            return "::1"
+        # Other kinds of errors MUST be raised so we can inspect
+        raise
+
+
 class _FakeServer(asyncio.StreamReaderProtocol):
     """
     Returned by _factory_invoker() in lieu of an SMTP instance in case
@@ -40,6 +80,7 @@ class Controller:
     server: Optional[AsyncServer] = None
     server_coro: Coroutine = None
     smtpd = None
+    _factory_invoked: Optional[threading.Event] = None
     _thread: Optional[threading.Thread] = None
     _thread_exception: Optional[Exception] = None
 
@@ -63,18 +104,19 @@ def __init__(
 /docs/controller.html#controller-api>`_.
         """
         self.handler = handler
-        self.hostname = "::1" if hostname is None else hostname
+        self.hostname = get_localhost() if hostname is None else hostname
         self.port = port
         self.ssl_context = ssl_context
         self.loop = asyncio.new_event_loop() if loop is None else loop
-        self.ready_timeout = os.getenv(
-            'AIOSMTPD_CONTROLLER_TIMEOUT', ready_timeout)
+        self.ready_timeout = float(
+            os.getenv("AIOSMTPD_CONTROLLER_TIMEOUT", ready_timeout)
+        )
         if server_kwargs:
             warn(
                 "server_kwargs will be removed in version 2.0. "
                 "Just specify the keyword arguments to forward to SMTP "
                 "as kwargs to this __init__ method.",
-                DeprecationWarning
+                DeprecationWarning,
             )
         self.SMTP_kwargs: Dict[str, Any] = server_kwargs or {}
         self.SMTP_kwargs.update(SMTP_parameters)
@@ -87,9 +129,7 @@ def __init__(
 
     def factory(self):
         """Allow subclasses to customize the handler/server creation."""
-        return SMTP(
-            self.handler, **self.SMTP_kwargs
-        )
+        return SMTP(self.handler, **self.SMTP_kwargs)
 
     def _factory_invoker(self):
         """Wraps factory() to catch exceptions during instantiation"""
@@ -101,6 +141,8 @@ def _factory_invoker(self):
         except Exception as err:
             self._thread_exception = err
             return _FakeServer(self.loop)
+        finally:
+            self._factory_invoked.set()
 
     def _run(self, ready_event):
         asyncio.set_event_loop(self.loop)
@@ -116,9 +158,7 @@ def _run(self, ready_event):
                 ssl=self.ssl_context,
             )
             self.server_coro = srv_coro
-            srv: AsyncServer = self.loop.run_until_complete(
-                srv_coro
-            )
+            srv: AsyncServer = self.loop.run_until_complete(srv_coro)
             self.server = srv
         except Exception as error:  # pragma: on-wsl
             # Usually will enter this part only if create_server() cannot bind to the
@@ -143,43 +183,63 @@ def _testconn(self):
         Context if necessary, and read some data from it to ensure that factory()
         gets invoked.
         """
+        # IMPORTANT: Windows does not need the next line; for some reasons,
+        # create_connection is happy with hostname="" on Windows, but screams murder
+        # in Linux.
+        # At this point, if self.hostname is Falsy, it most likely is "" (bind to all
+        # addresses). In such case, it should be safe to connect to localhost)
+        hostname = self.hostname or get_localhost()
         with ExitStack() as stk:
-            s = stk.enter_context(create_connection((self.hostname, self.port), 1.0))
+            s = stk.enter_context(create_connection((hostname, self.port), 1.0))
             if self.ssl_context:
                 s = stk.enter_context(self.ssl_context.wrap_socket(s))
             _ = s.recv(1024)
 
     def start(self):
         assert self._thread is None, "SMTP daemon already running"
+        self._factory_invoked = threading.Event()
+
         ready_event = threading.Event()
         self._thread = threading.Thread(target=self._run, args=(ready_event,))
         self._thread.daemon = True
         self._thread.start()
         # Wait a while until the server is responding.
-        ready_event.wait(self.ready_timeout)
-        if self._thread_exception is not None:  # pragma: on-wsl
-            # See comment about WSL1.0 in the _run() method
-            assert self._thread is not None  # Stupid LGTM.com; see github/codeql#4918
-            raise self._thread_exception
+        start = time.monotonic()
+        if not ready_event.wait(self.ready_timeout):
+            # An exception within self._run will also result in ready_event not set
+            # So, we first test for that, before raising TimeoutError
+            if self._thread_exception is not None:  # pragma: on-wsl
+                # See comment about WSL1.0 in the _run() method
+                raise self._thread_exception
+            else:
+                raise TimeoutError("SMTP server failed to start within allotted time")
+        respond_timeout = self.ready_timeout - (time.monotonic() - start)
+
         # Apparently create_server invokes factory() "lazily", so exceptions in
         # factory() go undetected. To trigger factory() invocation we need to open
         # a connection to the server and 'exchange' some traffic.
         try:
             self._testconn()
-        except Exception:
-            # We totally don't care of exceptions experienced by _testconn,
+        except socket_timeout:
+            # We totally don't care of timeout experienced by _testconn,
             # which _will_ happen if factory() experienced problems.
             pass
+        except Exception:
+            # Raise other exceptions though
+            raise
+        if not self._factory_invoked.wait(respond_timeout):
+            raise TimeoutError("SMTP server not responding within allotted time")
         if self._thread_exception is not None:
             raise self._thread_exception
+
         # Defensive
         if self.smtpd is None:
             raise RuntimeError("Unknown Error, failed to init SMTP server")
 
     def _stop(self):
         self.loop.stop()
         try:
-            _all_tasks = asyncio.all_tasks
+            _all_tasks = asyncio.all_tasks  # pytype: disable=module-attr
         except AttributeError:  # pragma: py-gt-36
             _all_tasks = asyncio.Task.all_tasks
         for task in _all_tasks(self.loop):

aiosmtpd/docs/NEWS.rst

@@ -3,6 +3,15 @@
 ===================
 
 
+1.3.1 (aiosmtpd-next)
+=====================
+
+Fixed/Improved
+==============
+* ``ready_timeout`` now actually enforced, raising ``TimeoutError`` if breached
+* No longer fail with opaque "Unknown Error" if ``hostname=""`` (Fixes #244)
+
+
 1.3.0 (2021-02-09)
 ==================
 

aiosmtpd/docs/controller.rst

@@ -220,6 +220,8 @@ Controller API
       a float number of seconds,
       which takes precedence over the ``ready_timeout`` argument value.
 
+      If this timeout is breached, a :class:`TimeoutError` exception will be raised.
+
    .. py:attribute:: ssl_context
       :type: ssl.SSLContext
 

aiosmtpd/smtp.py

@@ -160,17 +160,15 @@ def __init__(self, loop):
     @property
     def login_data(self):
         """Legacy login_data, usually containing the username"""
-        warn(
+        log.warning(
             "Session.login_data is deprecated and will be removed in version 2.0",
-            DeprecationWarning
         )
         return self._login_data
 
     @login_data.setter
     def login_data(self, value):
-        warn(
+        log.warning(
             "Session.login_data is deprecated and will be removed in version 2.0",
-            DeprecationWarning
         )
         self._login_data = value