Implement Unix Socket Controller #247

pepoluan · 2021-02-12T14:19:40Z

What do these changes do?

Add a new controller class, UnixSocketController, that listens on a Unix socket file instead of IP:port

Are there changes in behavior for the user?

Due to decomposition of Controller into BaseThreadedController + (pared down) Controller, user expecting Controller to not have a superclass is in for a surprise.

Other than that, there should be no problems.

Related issue number

Closes #114

Checklist

Split the BaseThreadedController logic from Controller, and make BaseThreadedController an Abstract Base Class. This is in preparation of implementing the UnixSocketController, which shares lots of logic, but not quite, with Controller.

Also exclude this class from testing if on Windows.

Inspired by aio-libs#244

Too noisy. log.warning is more subtle but still provide ample warning.

So we can ensure that it works. A test case accompanies this change.

Because that is its purpose: To trigger creation of the SMTP server.

* BaseThreadedController is its own entry * Controller reduced to just the difference with BaseThreadedController * UnixSocketController added in same vein

Because pytest's doctest runner can't conditionally skip on blocks.

* Indentation: the ":skipif:" option must be aligned with the doctest * AF_UNIX instead of AF_INET * Grab first response after connecting * Some proper SMTP exchange * Closing code * And replace a blank line with >>>

* Remove unused things * Update Sphinx requirement

I forgot that the "unix_socket" is a different group, and thus have different imports, classes, etc. Was not visible in Windows because it's skipped, but it exploded on Linux So rather than using `ExampleHandler` (which was a lengthy custom class) , we simply use Sink instead.

While we're at it, let's "decorate" that lone controller.stop() line.

Now that we run sphinx-build directly and not through setup.py, let's add the "--color" options to make life more colorful :)

pepoluan

Here are some comments for some possibly interesting points...

pepoluan · 2021-02-12T14:20:38Z

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

-          pytest -v aiosmtpd/docs
-          python setup.py build_sphinx
+          sphinx-build -b doctest -d build/.doctree aiosmtpd/docs build/doctest
+          sphinx-build -b html    -d build/.doctree aiosmtpd/docs build/html


pytest doctest can't conditionally skip blocks. Which we need to do on Win32.

So we now use Sphinx' own sphinx.ext.doctest.

pepoluan · 2021-02-12T14:22:14Z

aiosmtpd/controller.py

-            srv_coro: Coroutine = self.loop.create_server(
-                self._factory_invoker,
-                host=self.hostname,
-                port=self.port,
-                ssl=self.ssl_context,
-            )


Shifted to self._create_server to allow different methods of listening.

pepoluan · 2021-02-12T14:23:21Z

aiosmtpd/controller.py

+        if not ready_event.is_set():
+            raise TimeoutError("SMTP server failed to start within allotted time")


Previously we just wait and didn't actually do anything if timeout is reached.

This is now fixed, inspired by #244

pepoluan · 2021-02-12T14:24:09Z

aiosmtpd/controller.py

+class UnixSocketController(BaseThreadedController):  # pragma: on-win32
+    def __init__(
+        self,
+        handler,
+        unix_socket: Optional[Union[str, Path]],
+        loop=None,
+        *,
+        ready_timeout=1.0,
+        ssl_context=None,
+        # SMTP parameters
+        server_hostname: str = None,
+        **SMTP_parameters,
+    ):
+        super().__init__(
+            handler,
+            loop,
+            ready_timeout=ready_timeout,
+            ssl_context=ssl_context,
+            server_hostname=server_hostname,
+            **SMTP_parameters
+        )
+        self.unix_socket = str(unix_socket)
+
+    def _create_server(self) -> Coroutine:
+        return self.loop.create_unix_server(
+            self._factory_invoker,
+            path=self.unix_socket,
+            ssl=self.ssl_context,
+        )
+
+    def _trigger_server(self):
+        with ExitStack() as stk:
+            s: socket = stk.enter_context(socket(AF_UNIX, SOCK_STREAM))
+            s.connect(self.unix_socket)
+            if self.ssl_context:
+                s = stk.enter_context(self.ssl_context.wrap_socket(s))
+            _ = s.recv(1024)


New class! The gist of Issue #114

It's amazing how simple the diffcode is after the shared code is extracted to BaseThreadedController

pepoluan · 2021-02-12T14:25:00Z

aiosmtpd/docs/conf.py

@@ -51,21 +47,23 @@ def syspath_insert(pth: Path):

 # autoprogramm needs Sphinx>=1.2.2
 # :classmethod: needs Sphinx>=2.1
-needs_sphinx = "2.1"
+# :noindex: needs Sphinx>=3.2
+needs_sphinx = "3.2"


RTD uses 3.3.1 so this is still safe.

pepoluan · 2021-02-12T14:27:32Z

aiosmtpd/tests/test_server.py

+def in_win32():
+    return platform.system().casefold() == "windows"


UnixSocketController is not testable on Windows 32.

pepoluan · 2021-02-12T14:28:21Z

aiosmtpd/docs/controller.rst

+.. doctest:: unix_socket
+    :skipif: in_win32


The :skipif: there is why I revert from using pytest doctest and change to using sphinx-build -b doctest instead.

pepoluan · 2021-02-12T14:29:17Z

aiosmtpd/tests/test_server.py

+    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()


Apparently we haven't test the loop behavior before...

Test it as well.

pepoluan · 2021-02-12T14:49:34Z

pyproject.toml

+# addopts = """--doctest-glob="*.rst" --strict-markers -rfEX"""
+addopts = """--strict-markers -rfEX"""


We're now using Sphinx's sphinx.ext.doctest which is more flexible.

pepoluan · 2021-02-12T14:50:25Z

tox.ini

+    sphinx-build --color -b doctest -d build/.doctree aiosmtpd/docs build/doctest
+    sphinx-build --color -b html    -d build/.doctree aiosmtpd/docs build/html


This makes things a bit faster, actually, since the doctest builder and the html builder shared the same doctree.

Also, add expected message for TimeoutError

codecov · 2021-02-12T15:00:40Z

Codecov Report

Merging #247 (db6ee28) into master (be24d86) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #247   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1571      1606   +35     
  Branches       293       297    +4     
=========================================
+ Hits          1571      1606   +35

Impacted Files	Coverage Δ
aiosmtpd/smtp.py	`100.00% <ø> (ø)`
aiosmtpd/__init__.py	`100.00% <100.00%> (ø)`
aiosmtpd/controller.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be24d86...db6ee28. Read the comment docs.

pepoluan · 2021-02-12T15:10:40Z

Ugh, got hit by "AF_UNIX path too long" issue on MacOS 😑

Same issue as aio-libs/aiohttp#3572

Their fix is a bit too complex for my liking, though ... Let me write a simpler fixture for aiosmtpd 😉

See aio-libs/aiohttp#3572

We can now skip the assert; useful for exception handling in which stop() needs to be manually invoked, but thread possibly hasn't started yet. Also, added some ref-cleaning to assist gc. Finally, we added some test cases.

By setting its scope to "module", so it will be called -- and cleaned up -- exactly once.

pepoluan · 2021-02-15T18:25:36Z

aiosmtpd/controller.py

+        if not self._factory_invoked.wait(self.ready_timeout):
+            raise TimeoutError("SMTP server not responding within allotted time")
        if self._thread_exception is not None:
            raise self._thread_exception


If for some reasons factory() or _factory_invoker() take their sweet time in doing their job, we might miss a "late exception" because when main thread progresses to if self._thread_exception line there, _factory_invoker() might not have the opportunity yet to set the self._thread_exception variable.

Ideally, the timeout should be "what's left after waiting for create_server()", but that adds more complexity and I'm reluctant to do that.

pepoluan · 2021-02-15T18:26:43Z

aiosmtpd/controller.py

+    def stop(self, no_assert=False):
+        assert no_assert or self._thread is not None, "SMTP daemon not running"
        self.loop.call_soon_threadsafe(self._stop)
-        self._thread.join()
-        self._thread = None
+        if self._thread is not None:
+            self._thread.join()
+            self._thread = None


We now can bypass the assert test. Helpful for external exception handling because at that time, thread is possibly not started yet.

pepoluan · 2021-02-15T18:27:47Z

aiosmtpd/tests/test_server.py

+    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()


Despite the HUGE multiplier (total is 6), this does not affect test speed because test speed depends solely on the ready_timeout parameter.

pepoluan · 2021-02-17T05:04:11Z

If no issues, let's merge this by Wednesday (2021-02-17)

Due to the urgent bugfix of #248, we'll squamerge this not earlier than Friday (2021-02-19). To give time to marinate this PR after merging the changed master into this PR.

[skip ci]

# Conflicts: # aiosmtpd/__init__.py # aiosmtpd/controller.py # aiosmtpd/docs/NEWS.rst # aiosmtpd/docs/controller.rst # aiosmtpd/tests/test_server.py

# Conflicts: # aiosmtpd/__init__.py # aiosmtpd/docs/controller.rst

# Conflicts: # .github/workflows/unit-testing-and-coverage.yml # aiosmtpd/docs/NEWS.rst # aiosmtpd/docs/conf.py # tox.ini

Piecemeal from 0578d84, 00cc21e, 503cf47, 228409d, 31df5ef, ed641b9, c381061

Artefact due to merging with master. After all, we have changed to using sphinx-build doctest (5156b4d)

In accordance with the Developer's Guidelines: https://github.com/aio-libs/aiosmtpd/wiki/Configuration Also, fix a typo in tox.ini

Cygwin's AF_UNIX is emulated using AF_INET, and there are quirks that will cause problems with how we use AF_UNIX. So we disable the doctest on Cygwin as well, disable the whole Test Class, and exclude the UnixSocketController from coverage.

# Conflicts: # .gitignore # aiosmtpd/docs/NEWS.rst # pyproject.toml # tox.ini

pepoluan · 2021-02-24T10:15:45Z

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

-          pytest -v aiosmtpd/docs
-          sphinx-build --color -b html -d build/.doctree aiosmtpd/docs build/html
-          sphinx-build --color -b man  -d build/.doctree aiosmtpd/docs build/man
+          sphinx-build --color -b doctest -d build/.doctree aiosmtpd/docs build/doctest


We need selective doctest (UnixSocketController doesn't work on Windows). pytest can't do that, but sphinx-build can.

A side benefit: No longer receive the DeprecationWarning warning :-)

pepoluan · 2021-02-24T10:16:28Z

aiosmtpd/controller.py

+            _ = s.recv(1024)
+
+
+class UnixSocketController(BaseThreadedController):  # pragma: on-win32 on-cygwin


This class doesn't work properly on Windows and/or Cygwin.

pepoluan · 2021-02-24T10:17:10Z

aiosmtpd/docs/conf.py

+doctest_global_setup = """
+import sys
+in_win32 = sys.platform == "win32"
+in_cygwin = sys.platform == "cygwin"
+"""


Need to check for Cygwin as well (UnixSocketController does not work properly on Cygwin)

pepoluan · 2021-02-24T10:18:28Z

aiosmtpd/tests/test_server.py

+@pytest.fixture(scope="module")
+def safe_socket_dir() -> Generator[Path, None, None]:
+    # See:
+    #   - https://github.com/aio-libs/aiohttp/issues/3572
+    #   - https://github.com/aio-libs/aiohttp/pull/3832/files
+    #   - https://unix.stackexchange.com/a/367012/5589
+    tmpdir = Path(mkdtemp()).absolute()
+    assert len(str(tmpdir)) <= 87  # 92 (max on HP-UX) minus 5 (allow 4-char fn)
+    #
+    yield tmpdir
+    #
+    plist = [p for p in tmpdir.rglob("*")]
+    for p in reversed(plist):
+        if p.is_dir():
+            p.rmdir()
+        else:
+            p.unlink()
+    tmpdir.rmdir()


To create socket successfully in MacOS. The GA runner path is too deep on MacOS.

pepoluan · 2021-02-24T10:19:34Z

tox.ini

-[flake8]
-jobs = 1
-max-line-length = 88
-ignore = E123, E133, W503, W504, W293, E203


Moved to setup.cfg. See wiki for reasoning.

pepoluan · 2021-02-25T06:47:00Z

As soon as this last commit passes green, I'll squamerge.

Also use sed instead of grep

pepoluan added 19 commits February 12, 2021 16:45

Decompose Controller

4a172b8

Split the BaseThreadedController logic from Controller, and make BaseThreadedController an Abstract Base Class. This is in preparation of implementing the UnixSocketController, which shares lots of logic, but not quite, with Controller.

Implement UnixSocketController

0b1bd3d

Also exclude this class from testing if on Windows.

Test newly-implemented TimeoutError

c08fac7

Inspired by aio-libs#244

Add simple test for UnixSocketController

a5805a9

Add simple test for UnixSocketController with SSL

41c6710

Shorten some timeouts

d254e52

Replace DeprecationWarning for Session.login_data to log.warning

8ae916a

Too noisy. log.warning is more subtle but still provide ample warning.

Test connection to Unix Socket aio-libs#8

972c2a0

Also test that unix socket is truly an SMTP server

efbe415

Unroll decision on whether to create new event loop

b76a5e5

So we can ensure that it works. A test case accompanies this change.

Rename _test_server to _trigger_server

6ac1cd6

Because that is its purpose: To trigger creation of the SMTP server.

Reorder Controller args

245d166

Update documentation

c40d8cc

* BaseThreadedController is its own entry * Controller reduced to just the difference with BaseThreadedController * UnixSocketController added in same vein

Change doctest runner to Sphinx.ext.doctest

5156b4d

Because pytest's doctest runner can't conditionally skip on blocks.

Add conditonal doctest for UnixSocketController #1

366851e

Fixes for controller.rst

6157bae

* Indentation: the ":skipif:" option must be aligned with the doctest * AF_UNIX instead of AF_INET * Grab first response after connecting * Some proper SMTP exchange * Closing code * And replace a blank line with >>>

Also use sphinx-build doctest & html in GA

47fa8f1

Also install flake8-bugbear for the flake8 step in GA

1ab94cf

Update conf.py

01a0b8b

* Remove unused things * Update Sphinx requirement

pepoluan added the enhancement label Feb 12, 2021

pepoluan added this to the 1.4 milestone Feb 12, 2021

pepoluan added 3 commits February 12, 2021 21:40

Add filler text

985d670

While we're at it, let's "decorate" that lone controller.stop() line.

Add --color to sphinx-build

43a7d9a

Now that we run sphinx-build directly and not through setup.py, let's add the "--color" options to make life more colorful :)

pepoluan commented Feb 12, 2021

View reviewed changes

Remove "printer" fixture

ec8528e

Also, add expected message for TimeoutError

pepoluan force-pushed the socket-controller branch from b84fe6c to ec8528e Compare February 12, 2021 14:57

Add fixture for "safe" socket path

30ec66b

See aio-libs/aiohttp#3572

pepoluan added 4 commits February 16, 2021 01:18

Improve Controller.stop()

f735504

We can now skip the assert; useful for exception handling in which stop() needs to be manually invoked, but thread possibly hasn't started yet. Also, added some ref-cleaning to assist gc. Finally, we added some test cases.

Optimize safe_socket_dir fixture

c618306

By setting its scope to "module", so it will be called -- and cleaned up -- exactly once.

Merge branch 'master' into socket-controller

09d6f7e

Remove unneeded f

fa36d23

pepoluan commented Feb 15, 2021

View reviewed changes

pepoluan mentioned this pull request Feb 17, 2021

Controller.start uses threading #160

Closed

pepoluan added 5 commits February 17, 2021 17:04

Update in-commend link to documentation on RTD

5dc1d24

[skip ci]

Merge branch 'master' into socket-controller

01d4e5a

# Conflicts: # aiosmtpd/__init__.py # aiosmtpd/controller.py # aiosmtpd/docs/NEWS.rst # aiosmtpd/docs/controller.rst # aiosmtpd/tests/test_server.py

Fix Merge Failures

c66ea6b

Finish Merge leftovers

214aec2

Merge branch 'master' into socket-controller

2ef7e38

# Conflicts: # aiosmtpd/__init__.py # aiosmtpd/docs/controller.rst

pepoluan force-pushed the socket-controller branch from d778c05 to 2ef7e38 Compare February 19, 2021 19:28

pepoluan added the API label Feb 22, 2021

pepoluan added 9 commits February 23, 2021 16:22

Merge branch 'master' into socket-controller

d6dfccc

# Conflicts: # .github/workflows/unit-testing-and-coverage.yml # aiosmtpd/docs/NEWS.rst # aiosmtpd/docs/conf.py # tox.ini

Change _dynamic to _dump

436ceeb

Piecemeal from 0578d84, 00cc21e, 503cf47, 228409d, 31df5ef, ed641b9, c381061

Activate "static" with win32 exclusion

795fc8b

Remove pytest doctest

8622d64

Artefact due to merging with master. After all, we have changed to using sphinx-build doctest (5156b4d)

Move flake8 config to setup.cfg

144528f

In accordance with the Developer's Guidelines: https://github.com/aio-libs/aiosmtpd/wiki/Configuration Also, fix a typo in tox.ini

Add region..endregion fold marker

6ac5a20

Make "static" not run on cygwin as well

8ccb061

Merge branch 'master' into socket-controller

89afc94

# Conflicts: # .gitignore # aiosmtpd/docs/NEWS.rst # pyproject.toml # tox.ini

pepoluan commented Feb 24, 2021

View reviewed changes

Bump version to 1.4.0a5

bdfd13d

pepoluan added 2 commits February 25, 2021 14:02

Enforce different version in PR

b00bed9

Fix wrong equality test

db6ee28

Also use sed instead of grep

pepoluan merged commit 0da45f7 into aio-libs:master Feb 25, 2021

pepoluan deleted the socket-controller branch February 26, 2021 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Unix Socket Controller #247

Implement Unix Socket Controller #247

pepoluan commented Feb 12, 2021 •

edited

Loading

pepoluan left a comment

pepoluan Feb 12, 2021 •

edited

Loading

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

pepoluan Feb 12, 2021

codecov bot commented Feb 12, 2021 •

edited

Loading

pepoluan commented Feb 12, 2021

pepoluan Feb 15, 2021

pepoluan Feb 15, 2021

pepoluan Feb 15, 2021

pepoluan commented Feb 17, 2021

pepoluan Feb 24, 2021

pepoluan Feb 24, 2021

pepoluan Feb 24, 2021

pepoluan Feb 24, 2021

pepoluan Feb 24, 2021

pepoluan commented Feb 25, 2021

		if not ready_event.is_set():
		raise TimeoutError("SMTP server failed to start within allotted time")

		def in_win32():
		return platform.system().casefold() == "windows"

		# addopts = """--doctest-glob="*.rst" --strict-markers -rfEX"""
		addopts = """--strict-markers -rfEX"""

		sphinx-build --color -b doctest -d build/.doctree aiosmtpd/docs build/doctest
		sphinx-build --color -b html -d build/.doctree aiosmtpd/docs build/html

		_ = s.recv(1024)


		class UnixSocketController(BaseThreadedController): # pragma: on-win32 on-cygwin

Implement Unix Socket Controller #247

Implement Unix Socket Controller #247

Conversation

pepoluan commented Feb 12, 2021 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

pepoluan left a comment

Choose a reason for hiding this comment

pepoluan Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2021 • edited Loading

Codecov Report

pepoluan commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoluan commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoluan commented Feb 25, 2021

pepoluan commented Feb 12, 2021 •

edited

Loading

pepoluan Feb 12, 2021 •

edited

Loading

codecov bot commented Feb 12, 2021 •

edited

Loading