Repository: aio-libs/aiosmtpd (python)

Issue:
RuntimeError after upgrading from 1.2.2
After upgrading from version 1.2.2 to 1.2.4 we receive the following error when trying to start the controller:

```
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.6/site-packages/aiosmtpd/controller.py", line 165, in start
    raise RuntimeError("Unknown Error, failed to init SMTP server")
RuntimeError: Unknown Error, failed to init SMTP server
```

This error can even be triggered by running the sample code from the [documentation](https://aiosmtpd.readthedocs.io/en/latest/controller.html):

```
import asyncio
class ExampleHandler:
  async def handle_RCPT(self, server, session, envelope, address, rcpt_options):
    return '250 OK'

from aiosmtpd.controller import Controller
controller = Controller(ExampleHandler())
controller.start()
```
After executing "controller.start()" the RuntimeError from above appears.

Python version is 3.6.8
OS is Red Hat 7.9
aiosmtpd was installed with pip3. After downgrading via "sudo pip3 install aiosmtpd=1.2.2" everything works again and aiosmtpd receives and processes emails without problems.

Pull request:
Fix Controller Bug on hostname="" or IPv6 not available
<!-- Thank you for your contribution! -->

## What do these changes do?

Previously, `Controller._testconn()`, on Linux, will raise an error if `self.hostname=""`. This is due to `socket.create_connection()` not accepting `""` as hostname parameter. (Totally no problem in Windows)

In addition, `Controller.run()` wraps _all_ exceptions generated by `_testconn()` so this particular error is not seen, leading to hard times during troubleshooting.

Also, even if `hostname` not specified, `Controller` defaults to `"::1"`, which is not available on systems with IPv6 disabled.

Finally, previously `ready_timeout` is used only to wait, with no action taken if it's reached.

So, the changes:
* Now uses a smarter way to determine 'default localhost' address by attempting `::1`, and if that fails, `127.0.0.1`
* `ready_timeout` actually acts if timeout is breached
* Only swallow `socket.timeout` error but raises others

## Are there changes in behavior for the user?

User need to be aware that `ready_timeout` is no longer lip service; it is actually enforced with a possibility of raising a `TimeoutError` if violated.

Other than that, there should be no breaking behavioral changes.

## Related issue number

Fixes #244 

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [x] tox testenvs have been executed in the following environments:
   - [x] Windows 10 (via PyCharm tox runner): `(ALL)`
   - [x] Windows 10 (via Cygwin): `qa,py-{nocov,cov}`
   - [x] Ubuntu 18.04 on WSL 1.0: `(ALL+pypy3)`
   - [x] FreeBSD 12.2 on VBox: `(ALL+pypy3)`
- [x] Documentation reflects the changes
- [x] Add a news fragment into the `NEWS.rst` file


Patch:
diff --git a/aiosmtpd/__init__.py b/aiosmtpd/__init__.py
index 0689112..05ef765 100644
--- a/aiosmtpd/__init__.py
+++ b/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"
diff --git a/aiosmtpd/controller.py b/aiosmtpd/controller.py
index 27f1251..0a6d4e2 100644
--- a/aiosmtpd/controller.py
+++ b/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 @@ from aiosmtpd.smtp import SMTP
 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 @@ class Controller:
 /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 @@ class Controller:
 
     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 @@ class Controller:
         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 @@ class Controller:
                 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,35 +183,55 @@ class Controller:
         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")
@@ -179,7 +239,7 @@ class Controller:
     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):
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
index 41c505f..835a9f6 100644
--- a/aiosmtpd/docs/NEWS.rst
+++ b/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)
 ==================
 
diff --git a/aiosmtpd/docs/controller.rst b/aiosmtpd/docs/controller.rst
index 97aa875..1a58ec6 100644
--- a/aiosmtpd/docs/controller.rst
+++ b/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
 
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index bef3cb4..0d42f3d 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -160,17 +160,15 @@ class Session:
     @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
 
diff --git a/release.py b/release.py
index 4f96b6d..e3fd646 100755
--- a/release.py
+++ b/release.py
@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: Apache-2.0
 
 import os
+import re
 import subprocess
 import sys
 from pathlib import Path
@@ -45,6 +46,19 @@ GPG_SIGNING_ID = {GPG_SIGNING_ID or 'None'}
 choice = input(f"Release aiosmtpd {version} - correct? [y/N]: ")
 if choice.lower() not in ("y", "yes"):
     sys.exit("Release aborted")
+
+newsfile = Path(".") / "aiosmtpd" / "docs" / "NEWS.rst"
+with newsfile.open("rt") as fin:
+    want = re.compile("^" + re.escape(version) + r"\s*\(\d{4}-\d\d-\d\d\)")
+    for ln in fin:
+        m = want.match(ln)
+        if not m:
+            continue
+        break
+    else:
+        print(f"ERROR: I found no datestamped entry for {version} in NEWS.rst!")
+        sys.exit(1)
+
 if not GPG_SIGNING_ID:
     choice = input("You did not specify GPG signing ID! Continue? [y/N]: ")
     if choice.lower() not in ("y", "yes"):


Test patch:
diff --git a/.github/workflows/unit-testing-and-coverage.yml b/.github/workflows/unit-testing-and-coverage.yml
index 6e850b2..6d77512 100644
--- a/.github/workflows/unit-testing-and-coverage.yml
+++ b/.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:
diff --git a/aiosmtpd/tests/test_server.py b/aiosmtpd/tests/test_server.py
index 2572450..96ced4a 100644
--- a/aiosmtpd/tests/test_server.py
+++ b/aiosmtpd/tests/test_server.py
@@ -3,20 +3,53 @@
 
 """Test other aspects of the server implementation."""
 
+import errno
 import platform
 import socket
+import time
 from functools import partial
 
 import pytest
 from pytest_mock import MockFixture
 
-from aiosmtpd.controller import Controller, _FakeServer
+from aiosmtpd.controller import Controller, _FakeServer, get_localhost
 from aiosmtpd.handlers import Sink
 from aiosmtpd.smtp import SMTP as Server
 
 from .conftest import Global
 
 
+class SlowStartController(Controller):
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault("ready_timeout", 0.5)
+        super().__init__(*args, **kwargs)
+
+    def _run(self, ready_event):
+        time.sleep(self.ready_timeout * 1.5)
+        try:
+            super()._run(ready_event)
+        except Exception:
+            pass
+
+
+class SlowFactoryController(Controller):
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault("ready_timeout", 0.5)
+        super().__init__(*args, **kwargs)
+
+    def factory(self):
+        time.sleep(self.ready_timeout * 3)
+        return super().factory()
+
+    def _factory_invoker(self):
+        time.sleep(self.ready_timeout * 3)
+        return super()._factory_invoker()
+
+
+def in_win32():
+    return platform.system().casefold() == "windows"
+
+
 def in_wsl():
     # WSL 1.0 somehow allows more than one listener on one port.
     # So we have to detect when we're running on WSL so we can skip some tests.
@@ -56,6 +89,35 @@ class TestServer:
 class TestController:
     """Tests for the aiosmtpd.controller.Controller class"""
 
+    @pytest.mark.filterwarnings("ignore")
+    def test_ready_timeout(self):
+        cont = SlowStartController(Sink())
+        expectre = r"SMTP server failed to start within allotted time"
+        try:
+            with pytest.raises(TimeoutError, match=expectre):
+                cont.start()
+        finally:
+            cont.stop()
+
+    @pytest.mark.filterwarnings("ignore")
+    def test_factory_timeout(self):
+        cont = SlowFactoryController(Sink())
+        expectre = r"SMTP server not responding within allotted time"
+        try:
+            with pytest.raises(TimeoutError, match=expectre):
+                cont.start()
+        finally:
+            cont.stop()
+
+    def test_reuse_loop(self, temp_event_loop):
+        cont = Controller(Sink(), loop=temp_event_loop)
+        assert cont.loop is temp_event_loop
+        try:
+            cont.start()
+            assert cont.smtpd.loop is temp_event_loop
+        finally:
+            cont.stop()
+
     @pytest.mark.skipif(in_wsl(), reason="WSL prevents socket collision")
     def test_socket_error_dupe(self, plain_controller, client):
         contr2 = Controller(
@@ -129,6 +191,72 @@ class TestController:
         controller = contsink(server_hostname="testhost3", server_kwargs=kwargs)
         assert controller.SMTP_kwargs["hostname"] == "testhost3"
 
+    def test_hostname_empty(self):
+        # WARNING: This test _always_ succeeds in Windows.
+        cont = Controller(Sink(), hostname="")
+        try:
+            cont.start()
+        finally:
+            cont.stop()
+
+    def test_hostname_none(self):
+        cont = Controller(Sink())
+        try:
+            cont.start()
+        finally:
+            cont.stop()
+
+    def test_testconn_raises(self, mocker: MockFixture):
+        mocker.patch("socket.socket.recv", side_effect=RuntimeError("MockError"))
+        cont = Controller(Sink(), hostname="")
+        try:
+            with pytest.raises(RuntimeError, match="MockError"):
+                cont.start()
+        finally:
+            cont.stop()
+
+    def test_getlocalhost(self):
+        assert get_localhost() in ("127.0.0.1", "::1")
+
+    def test_getlocalhost_noipv6(self, mocker):
+        mock_hasip6 = mocker.patch("aiosmtpd.controller._has_ipv6", return_value=False)
+        assert get_localhost() == "127.0.0.1"
+        assert mock_hasip6.called
+
+    def test_getlocalhost_6yes(self, mocker: MockFixture):
+        mock_sock = mocker.Mock()
+        mock_makesock: mocker.Mock = mocker.patch("aiosmtpd.controller.makesock")
+        mock_makesock.return_value.__enter__.return_value = mock_sock
+        assert get_localhost() == "::1"
+        mock_makesock.assert_called_with(socket.AF_INET6, socket.SOCK_STREAM)
+        assert mock_sock.bind.called
+
+    def test_getlocalhost_6no(self, mocker):
+        mock_makesock: mocker.Mock = mocker.patch(
+            "aiosmtpd.controller.makesock",
+            side_effect=OSError(errno.EADDRNOTAVAIL, "Mock IP4-only"),
+        )
+        assert get_localhost() == "127.0.0.1"
+        mock_makesock.assert_called_with(socket.AF_INET6, socket.SOCK_STREAM)
+
+    def test_getlocalhost_6inuse(self, mocker):
+        mock_makesock: mocker.Mock = mocker.patch(
+            "aiosmtpd.controller.makesock",
+            side_effect=OSError(errno.EADDRINUSE, "Mock IP6 used"),
+        )
+        assert get_localhost() == "::1"
+        mock_makesock.assert_called_with(socket.AF_INET6, socket.SOCK_STREAM)
+
+    def test_getlocalhost_error(self, mocker):
+        mock_makesock: mocker.Mock = mocker.patch(
+            "aiosmtpd.controller.makesock",
+            side_effect=OSError(errno.EAFNOSUPPORT, "Mock Error"),
+        )
+        with pytest.raises(OSError, match="Mock Error") as exc:
+            get_localhost()
+        assert exc.value.errno == errno.EAFNOSUPPORT
+        mock_makesock.assert_called_with(socket.AF_INET6, socket.SOCK_STREAM)
+
 
 class TestFactory:
     def test_normal_situation(self):
@@ -143,7 +271,7 @@ class TestFactory:
     @pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
     def test_unknown_args_direct(self, silence_event_loop_closed):
         unknown = "this_is_an_unknown_kwarg"
-        cont = Controller(Sink(), **{unknown: True})
+        cont = Controller(Sink(), ready_timeout=0.3, **{unknown: True})
         expectedre = r"__init__.. got an unexpected keyword argument '" + unknown + r"'"
         try:
             with pytest.raises(TypeError, match=expectedre):
@@ -159,7 +287,7 @@ class TestFactory:
     @pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
     def test_unknown_args_inkwargs(self, silence_event_loop_closed):
         unknown = "this_is_an_unknown_kwarg"
-        cont = Controller(Sink(), server_kwargs={unknown: True})
+        cont = Controller(Sink(), ready_timeout=0.3, server_kwargs={unknown: True})
         expectedre = r"__init__.. got an unexpected keyword argument '" + unknown + r"'"
         try:
             with pytest.raises(TypeError, match=expectedre):
@@ -173,7 +301,7 @@ class TestFactory:
         # Hypothetical situation where factory() did not raise an Exception
         # but returned None instead
         mocker.patch("aiosmtpd.controller.SMTP", return_value=None)
-        cont = Controller(Sink())
+        cont = Controller(Sink(), ready_timeout=0.3)
         expectedre = r"factory\(\) returned None"
         try:
             with pytest.raises(RuntimeError, match=expectedre):