Fix parsing of the `--bind` option for abstract UNIX sockets #248

cyraxjoe · 2019-11-13T22:40:17Z

❓ What kind of change does this PR introduce?

📋 What is the related issue number (starting with #)

It wasn't reported.

❓ What is the current behavior? (You can also link to an open issue here)

The --bind option wasn't properly parsed for abstract unix socket.
Something like cheroot --bind @mysocket would immediately fail with something like:

$ python -m cheroot 'myapp.wsgi' --bind @MYSOCK
Keyboard Interrupt: shutting down
Traceback (most recent call last):
  File "/nix/store/nzb7x6kij7mk07m88jwv8m1hhyjzjmb1-python3-3.8.0/lib/python3.8/runpy.py", line 192, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/nix/store/nzb7x6kij7mk07m88jwv8m1hhyjzjmb1-python3-3.8.0/lib/python3.8/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/nix/store/ij4qzqw81lnas4f59g05l3amkh928y4l-python3.8-cheroot-8.2.1/lib/python3.8/site-packages/cheroot/__main__.py", line 6, in <module>
    main()
  File "/nix/store/ij4qzqw81lnas4f59g05l3amkh928y4l-python3.8-cheroot-8.2.1/lib/python3.8/site-packages/cheroot/cli.py", line 234, in main
    raw_args._wsgi_app.server(raw_args).safe_start()
  File "/nix/store/ij4qzqw81lnas4f59g05l3amkh928y4l-python3.8-cheroot-8.2.1/lib/python3.8/site-packages/cheroot/server.py", line 1686, in safe_start
    self.start()
  File "/nix/store/ij4qzqw81lnas4f59g05l3amkh928y4l-python3.8-cheroot-8.2.1/lib/python3.8/site-packages/cheroot/server.py", line 1795, in start
    self.prepare()
  File "/nix/store/ij4qzqw81lnas4f59g05l3amkh928y4l-python3.8-cheroot-8.2.1/lib/python3.8/site-packages/cheroot/server.py", line 1728, in prepare
    info = socket.getaddrinfo(
  File "/nix/store/nzb7x6kij7mk07m88jwv8m1hhyjzjmb1-python3-3.8.0/lib/python3.8/socket.py", line 914, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
OSError: [Errno 16] Device or resource busy

Because the server would try to connect using TCP/IP at MYSOCK port None, given the faulty parsing of the CLI.

❓ What is the new behavior (if this is a feature change)?

The --bind option now works for abstract UNIX sockets.

📋 Other information:

📋 Checklist:

I think the code is well written
I wrote good commit messages
I have squashed related commits together after the changes have been approved
Unit tests for the changes exist
Integration tests for the changes exist (if applicable)
I used the same coding conventions as the rest of the project
The new code doesn't generate linter offenses
Documentation reflects the changes
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences

This change is

wsgi application parsing from the CLI. Hint: there is an issue with abstract unix sockets.

codecov · 2019-11-13T22:49:10Z

Codecov Report

Merging #248 into master will increase coverage by 1.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   76.82%   77.91%   +1.08%     
==========================================
  Files          24       25       +1     
  Lines        3806     3830      +24     
==========================================
+ Hits         2924     2984      +60     
+ Misses        882      846      -36

webknjaz · 2019-11-13T22:52:39Z

cheroot/test/test_cli.py

+)
+
+
+def test_parse_wsgi_bind_location_for_tcpip():


Could you plz use @pytest.mark.parametrize here?

webknjaz · 2019-11-13T22:53:50Z

cheroot/test/test_cli.py

+        def main(self):
+            pass
+    try:
+        wsgi_app_mock = WSGIAppMock()


Plz make a fixture providing this.

webknjaz · 2019-11-13T22:54:21Z

cheroot/test/test_cli.py

+            pass
+    try:
+        wsgi_app_mock = WSGIAppMock()
+        sys.modules['mypkg.wsgi'] = wsgi_app_mock


I think it's best to rely on the builtin monkeypatch fixture provided by pytest.

cheroot/cli.py

webknjaz · 2019-11-13T22:56:38Z

Hey Joel!

The change looks okay, but we need linters to pass and please use pytest's way of organizing tests as I outlined in the comments.

webknjaz · 2019-11-13T22:58:27Z

cheroot/test/test_cli.py

+    try:
+        wsgi_app_mock = WSGIAppMock()
+        sys.modules['mypkg.wsgi'] = wsgi_app_mock
+        app = Application.resolve('mypkg.wsgi')


Please use parametrize for this as well.

webknjaz · 2019-11-13T22:58:42Z

cheroot/test/test_cli.py

+
+
+def test_Aplication_resolve():
+    import sys


Imports like this belong to the top of the module. But maybe you could even avoid it with the use of monkeypatch fixture.

webknjaz · 2019-11-13T23:00:59Z

P.S. Running tox -e pre-commit (locally) does some minor bits of autoformatting automatically, so you can use it too.

cyraxjoe · 2019-11-13T23:11:54Z

Hey @webknjaz!

Thank you for your detailed review!

I'll make the changes later in the day. 👍

…ions.

cyraxjoe · 2019-11-14T22:46:15Z

😞 the idea of using sys.modules as a way to mock the module loading does not work in python 2. import_module is implemented differently, it uses __import__ without verifying sys.modules.

As a side effect... after running the test in python2, another error existed with the use of `contextlib.suppress`, `suppress` was introduced in python 3.4, so, to support both versions I replaced the context manager with the explicit verification of `inspect.isclass` instead of suppressing `TypeError`.

cyraxjoe · 2019-11-15T00:48:50Z

Note to self: mark the PR as WIP to avoid triggering all the verification unnecessarily. 😅

cheroot/cli.py

webknjaz · 2019-11-15T20:26:14Z

cheroot/cli.py

-        return cls(app)
+        # verify that `app` is a class before the issubclass verification,
+        # otherwise a TypeError will be rised.
+        if inspect.isclass(app) and issubclass(app, server.Gateway):


EAFP is more natural to Python. If you worry about the Py2 compat, I could cherry-pick this https://github.com/cherrypy/cheroot/pull/243/files#diff-544070cbdc8d4e10b3faf72ba309abc6R18-R31 shim to master.

Yeah the original approach was a plain try/except TypeError and ignore with a comment on why are we ignoring that exception, but I end up preferring the LBFL in this case. But it seem that the suppress context manager is used a lot in the other PR. If that's going to be included, then yeah I could use that here and be done by leaving this fragment as it was.

It's in master already.

webknjaz · 2019-11-15T20:29:09Z

cheroot/test/test_cli.py

+@pytest.mark.parametrize(
+    'raw_bind_addr, expected_bind_addr', (
+        ('192.168.1.1:80', ('192.168.1.1', 80)),
+        ('[::1]:8000', ('::1', 8000)),


This reminds me that we should probably also test catch-all addrs like

Suggested change

('[::1]:8000', ('::1', 8000)),

('[::1]:8000', ('::1', 8000)),

('[::]:8000', ('::', 8000)),

('12345', ('::', 12345)),

Yeah... I was just avoiding doing the extra work of thinking of future possible uses cases and all the possible values that urllib may support.

webknjaz · 2019-11-15T20:37:30Z

cheroot/test/test_cli.py

+)
+def test_parse_wsgi_bind_addr_for_tcpip(raw_bind_addr, expected_bind_addr):
+    """Check the parsing of the --bind option for TCP/IP addresses."""
+    assert parse_wsgi_bind_addr(raw_bind_addr) == expected_bind_addr


I'd merge these all similar tests into parametrize of the first one. Just make sure to use pytest.param(..., ..., id=) (http://doc.pytest.org/en/latest/example/parametrize.html#different-options-for-test-ids / https://docs.pytest.org/en/latest/reference.html#pytest-param)

webknjaz · 2019-11-15T20:40:02Z

cheroot/test/test_cli.py

+
+@pytest.mark.parametrize(
+    'wsgi_app_spec, pkg_name, app_method, mocked_app', (
+        ('mypkg.wsgi', 'mypkg.wsgi', 'application', WSGIAppMock()),


WSGIAppMock() is evaluated during the test module import time. Better use a fixture.

The mock instance is directly related to the other params. I did liked the idea of passing it along the others params to express the direct relationship.

For example a future param ('a.b:foo', 'a.b', 'foo') will break the test, because the mock doesn't define a foo method. This is not obvious via the implicit mock injection of pytest.

But I understand if you have a strong opinion on going over the pytest mock injection route.

In this case, you'd have to deal with this on the fixture level. This is a pytest way.

webknjaz · 2019-11-15T20:44:23Z

cheroot/test/test_cli.py

+    assert parse_wsgi_bind_addr('@cheroot') == '\0cheroot'
+
+
+class WSGIAppMock:


This is how you make a fixture with pytest:

Suggested change

class WSGIAppMock:

@pytest.fixture

def wsgi_app(monkeypatch):

"""Return a WSGI app stub."""

class WSGIAppMock:

"""Mock of a wsgi module."""

def application(self):

"""Empty application method.

Default method to be called when no specific callable

is defined in the wsgi application identifier.

It has an empty body because we are expecting to verify that

the same method is return no the actual execution of it.

"""

def main(self):

"""Empty custom method (callable) inside the mocked WSGI app.

It has an empty body because we are expecting to verify that

the same method is return no the actual execution of it.

"""

mocked_app = WSGIAppMock()

if six.PY2:

# python2 requires the previous namespaces to be part of sys.modules

# (e.g. for 'a.b.c' we need to insert 'a', 'a.b' and 'a.b.c')

# otherwise it fails, we're setting the same instance on each level,

# we don't really care about those, just the last one.

full_path, *pkg_parts, _last_part = pkg_name.split('.')

for p in pkg_parts:

full_path = '.'.join((full_path, p))

monkeypatch.setitem(sys.modules, full_path, mocked_app)

monkeypatch.setitem(sys.modules, pkg_name, mocked_app)

return mocked_app

BTW that unpacking syntax doesn't work on Python 2.

Wow, I guess I'm too addicted to py3 :)

webknjaz · 2019-11-15T20:44:59Z

cheroot/test/test_cli.py

+)
+def test_Aplication_resolve(
+    monkeypatch,
+    wsgi_app_spec, pkg_name, app_method, mocked_app,


To use the fixture from https://github.com/cherrypy/cheroot/pull/248/files#r347003745:

Suggested change

wsgi_app_spec, pkg_name, app_method, mocked_app,

wsgi_app_spec, pkg_name, app_method, wsgi_app,

webknjaz · 2019-11-15T21:08:41Z

cheroot/test/test_cli.py

+    wsgi_app_spec, pkg_name, app_method, mocked_app,
+):
+    """Check the wsgi application name conversion."""
+    if six.PY2:


This whole monkeypatching can be also moved to the fixture so that only testing remains in the test function: https://github.com/cherrypy/cheroot/pull/248/files#r347003745.

webknjaz · 2019-11-15T21:10:35Z

cheroot/test/test_core.py

-    # then this will result in an `OverflowError: Python int too large to convert to C ssize_t`
-    # in the server.
+    # then this will result in an `OverflowError: Python int too large to
+    # convert to C `ssize_t` in the server.


Wow, I probably missed this in the previous PR. I'll commit it to master directly because it's completely unrelated to this PR.

This one's in master already.

to make it python2 compatible using a EAFP style, instead of using `inspect.isclass`.

- Unify the bind_addr test into a single function parametrized by pytest. - Add additional parameters in the `test_parse_wsgi_bind_addr` function to further test some additional cases. - Encapsulate the WSGI mock definition, patching, and injection in the test using pytest conventions.

webknjaz · 2019-11-18T00:37:00Z

cheroot/test/test_cli.py

+        # this is a valid input, but foo gets discarted
+        ('foo@bar:5000', ('bar', 5000)),
+        ('foo', ('foo', None)),
+        ('123456789', ('123456789', None)),


How about :8080?

and catch-all addresses like 0.0.0.0/::/[::]?

webknjaz · 2019-11-18T00:44:24Z

cheroot/test/test_cli.py

+def test_parse_wsgi_bind_addr(raw_bind_addr, expected_bind_addr):
+    """Check the parsing of the --bind option.
+
+    Verify some of the supported addresses and the excpected return value.


It'd be nice to also have a negative test with invalid data. But that could be done separately.

webknjaz

It's good enough to be accepted. But feel free to follow up with more improvements :)

cyraxjoe added 3 commits November 13, 2019 16:26

Add some tests to verify the --bind parameter and

3fedfbe

wsgi application parsing from the CLI. Hint: there is an issue with abstract unix sockets.

Fix the --bind CLI option pasing for abstract unix sockets.

4158588

Fix flake8 complains about spacing of test_cli.py

a3eb351

cyraxjoe changed the title ~~Fix the parsing of the "--bind" option for abstract unix sockets.~~ Fix parsing of the "--bind" option for abstract unix sockets. Nov 13, 2019

webknjaz reviewed Nov 13, 2019

View reviewed changes

cheroot/test/test_cli.py Outdated

def main(self):

pass

try:

wsgi_app_mock = WSGIAppMock()

Copy link

Member

webknjaz Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz make a fixture providing this.

webknjaz reviewed Nov 13, 2019

View reviewed changes

cheroot/cli.py Show resolved Hide resolved

webknjaz reviewed Nov 13, 2019

View reviewed changes

webknjaz changed the title ~~Fix parsing of the "--bind" option for abstract unix sockets.~~ Fix parsing of the --bind option for abstract unix sockets Nov 13, 2019

cyraxjoe added 3 commits November 14, 2019 15:00

Modify test_cli.py to make it follow the pytest and sytlistic convent…

646477a

…ions.

Tiny change in test_core, so that the linter is happy.

20b32bd

Add some blank lines to give structure in parse_wsgi_bind_location

63325b5

cyraxjoe requested a review from webknjaz November 14, 2019 22:11

cyraxjoe added 2 commits November 14, 2019 18:20

Remove print call that was never meant to be committed.

7e37de1

webknjaz reviewed Nov 15, 2019

View reviewed changes

cheroot/cli.py Show resolved Hide resolved

webknjaz reviewed Nov 15, 2019

View reviewed changes

cyraxjoe added 2 commits November 17, 2019 11:19

Merge branch 'master' into fix-abstract-socket-cli

76be61e

Use the newly included _compat.suppress function

39bdf1e

to make it python2 compatible using a EAFP style, instead of using `inspect.isclass`.

cyraxjoe changed the title ~~Fix parsing of the --bind option for abstract unix sockets~~ [WIP] Fix parsing of the --bind option for abstract unix sockets Nov 17, 2019

cyraxjoe changed the title ~~[WIP] Fix parsing of the --bind option for abstract unix sockets~~ Fix parsing of the --bind option for abstract unix sockets Nov 17, 2019

webknjaz reviewed Nov 18, 2019

View reviewed changes

webknjaz approved these changes Nov 18, 2019

View reviewed changes

webknjaz changed the title ~~Fix parsing of the --bind option for abstract unix sockets~~ Fix parsing of the --bind option for abstract UNIX sockets Nov 18, 2019

webknjaz merged commit 7c6715e into cherrypy:master Nov 18, 2019

cyraxjoe deleted the fix-abstract-socket-cli branch August 21, 2020 19:38

		assert parse_wsgi_bind_addr('@cheroot') == '\0cheroot'


		class WSGIAppMock:

-class WSGIAppMock:
+@pytest.fixture
+def wsgi_app(monkeypatch):
+    """Return a WSGI app stub."""
+    class WSGIAppMock:
+	    """Mock of a wsgi module."""
+	    def application(self):
+	        """Empty application method.
+	        Default method to be called when no specific callable
+	        is defined in the wsgi application identifier.
+	        It has an empty body because we are expecting to verify that
+	        the same method is return no the actual execution of it.
+	        """
+	    def main(self):
+	        """Empty custom method (callable) inside the mocked WSGI app.
+	        It has an empty body because we are expecting to verify that
+	        the same method is return no the actual execution of it.
+	        """
+	mocked_app = WSGIAppMock()
+    if six.PY2:
+        # python2 requires the previous namespaces to be part of sys.modules
+        #   (e.g. for 'a.b.c' we need to insert 'a', 'a.b' and 'a.b.c')
+        # otherwise it fails, we're setting the same instance on each level,
+        # we don't really care about those, just the last one.
+        full_path, *pkg_parts, _last_part = pkg_name.split('.')
+        for p in pkg_parts:
+            full_path = '.'.join((full_path, p))
+            monkeypatch.setitem(sys.modules, full_path, mocked_app)
+    monkeypatch.setitem(sys.modules, pkg_name, mocked_app)
+	return mocked_app

	wsgi_app_spec, pkg_name, app_method, mocked_app,
	wsgi_app_spec, pkg_name, app_method, wsgi_app,

Uh oh!

Fix parsing of the --bind option for abstract UNIX sockets #248

Fix parsing of the --bind option for abstract UNIX sockets #248

Uh oh!

Conversation

cyraxjoe commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

webknjaz commented Nov 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyraxjoe commented Nov 13, 2019

Uh oh!

cyraxjoe commented Nov 14, 2019

Uh oh!

cyraxjoe commented Nov 15, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz left a comment

Fix parsing of the `--bind` option for abstract UNIX sockets #248

Fix parsing of the `--bind` option for abstract UNIX sockets #248

cyraxjoe commented Nov 13, 2019 •

edited

Loading

codecov bot commented Nov 13, 2019 •

edited

Loading

webknjaz commented Nov 13, 2019 •

edited

Loading

webknjaz Nov 15, 2019 •

edited

Loading

webknjaz Nov 15, 2019 •

edited

Loading

webknjaz Nov 18, 2019 •

edited

Loading