Add type hints to aioftp

[honglei]

Since the aioftp requires Python3.7+ right now, type hints may benefit the project.

[pohmelie]

How will they benefit the project? 🤔

[honglei]

@pohmelie https://realpython.com/python-type-checking/#pros-and-cons

[honglei]

https://fastapi.tiangolo.com/release-notes/#highlights https://python.plainenglish.io/how-to-use-the-new-and-cool-annotated-typing-feature-of-fastapi-4a2fdc48ef74

[pohmelie]

Ok, aioftp already have good docs with types in docstrings, aioftp do not use pydantic. If you have time to do PR I will review it and merge.

[honglei]

@pohmelie PR https://github.com/aio-libs/aioftp/pull/165

[9en9i]

Are there any updates? Is there anything I can do to help?

[pohmelie]

It looks like #165 is stuck a little. So it is up to you to make new pr.

[9en9i]

Okay, I'm on it. Is the target version of Python now 3.8? Previously as I see in the pr it was 3.11

[pohmelie]

Yep, I set to 3.11 before, but forget about old versions life cycle. So right now it is 3.8+.

[9en9i]

Hi, working on a pull request with added typing is nearing completion, but I've run into some issues. First I would like to know if a comparison of type analyzer tools has been done? I saw that mypy was discussed in a past pull request, is it still the choice? Or maybe there is an option to use pyright? My main type analyzer tool at work is pyright, so I decided to simplify things a bit and use a more familiar tool to start with. So, I used it to fix (actually most of it, the rest needs discussion because it goes beyond just type checking and I didn't dare touch it without discussion) all the typing errors and turned on mypy, assuming that there might be some minor differences that could be fixed. But I encountered missing functionality in mypy. I'm talking about this use case, which pyright implemented a few years ago: https://github.com/microsoft/pyright/discussions/2737. Because of the lack of possibility to use Literal with overload decorator mypy mistakenly finds about a hundred errors (maybe a little less). In my experience pyright is a more advanced analyzer because of faster implementation of features and as a consequence wider support.

Regarding mypy, I also think in this case I can fix the code using the protocol.

[pohmelie]

@9en9i

First I would like to know if a comparison of type analyzer tools has been done?

IDK which is better, but for me mypy is 99% ok. I'm using it in some projects. You can use any type checker you want, since there are no type linters right now.

[9en9i]

@pohmelie Hi, is there any information somewhere about what you need to run to run the tests?

[9en9i]

I mean I get a timeout error when running tests.

[pohmelie]

There are no «special» requirements for tests. You can check instructions in github workflows (ci-cd.yml file). But it is just a pip install -e ./[dev] and pytest.

[9en9i]

Yeah, that's what I did. But I get an error on test 45 FAILED tests/test_connection.py::test_pasv_connection_pasv_forced_response_address[127.0.0.1] - TimeoutError

[pohmelie]

Is there any traceback?

[9en9i]

/Users/denis/code/aioftp/.venv/bin/python3.8 /Users/denis/Applications/PyCharm Professional Edition.app/Contents/plugins/python/helpers/pycharm/_jb_pytest_runner.py --target test_connection.py::test_pasv_connection_pasv_forced_response_address 
Testing started at 15:15 ...
Launching pytest with arguments test_connection.py::test_pasv_connection_pasv_forced_response_address --no-header --no-summary -q in /Users/denis/code/aioftp/tests

============================= test session starts ==============================
collecting ... collected 778 items

test_abort.py::test_abort_stor[127.0.0.1] 
test_abort.py::test_abort_stor[::1] 
test_abort.py::test_abort_retr[127.0.0.1] 
test_abort.py::test_abort_retr[::1] 
test_abort.py::test_abort_retr_no_wait[127.0.0.1] 
test_abort.py::test_abort_retr_no_wait[::1] 
test_abort.py::test_nothing_to_abort[127.0.0.1] 
test_abort.py::test_nothing_to_abort[::1] 
test_abort.py::test_mlsd_abort[127.0.0.1] 
test_abort.py::test_mlsd_abort[::1] 
test_client_side_socks.py::test_socks_success[127.0.0.1-socks0] PASSED                         [  0%]PASSED                               [  0%]PASSED                         [  0%]PASSED                               [  0%]PASSED                 [  0%]PASSED                       [  0%]PASSED                   [  0%]PASSED                         [  1%]PASSED                         [  1%]PASSED                               [  1%]
test_client_side_socks.py::test_socks_success[127.0.0.1-socks1] 
test_client_side_socks.py::test_socks_success[::1-socks0] 
test_client_side_socks.py::test_socks_success[::1-socks1] 
test_client_side_socks.py::test_socks_fail[127.0.0.1-socks0] 
test_client_side_socks.py::test_socks_fail[127.0.0.1-socks1] 
test_client_side_socks.py::test_socks_fail[::1-socks0] 
test_client_side_socks.py::test_socks_fail[::1-socks1] 
test_connection.py::test_client_without_server[127.0.0.1] PASSED   [  1%]PASSED   [  1%]PASSED         [  1%]PASSED         [  1%]PASSED      [  1%]PASSED      [  2%]PASSED            [  2%]PASSED            [  2%]
test_connection.py::test_client_without_server[::1] 
test_connection.py::test_connection[127.0.0.1] 
test_connection.py::test_quit[::1] 
test_connection.py::test_not_implemented[127.0.0.1] 
test_connection.py::test_not_implemented[::1] 
test_connection.py::test_type_success[127.0.0.1] 
test_connection.py::test_type_success[::1] 
test_connection.py::test_custom_passive_commands[127.0.0.1] 
test_connection.py::test_custom_passive_commands[::1] 
test_connection.py::test_extra_pasv_connection[127.0.0.1] 
test_connection.py::test_extra_pasv_connection[::1] 
test_connection.py::test_closing_passive_connection[127.0.0.1-epsv] 
test_connection.py::test_closing_passive_connection[127.0.0.1-pasv] 
test_connection.py::test_closing_passive_connection[::1-epsv] 
test_connection.py::test_closing_passive_connection[::1-pasv] 
test_connection.py::test_pasv_connection_ports_not_added[127.0.0.1] 
test_connection.py::test_pasv_connection_ports_not_added[::1] 
test_connection.py::test_pasv_connection_ports[127.0.0.1] 
test_connection.py::test_pasv_connection_ports[::1] 
test_connection.py::test_data_ports_remains_empty[127.0.0.1] 
test_connection.py::test_data_ports_remains_empty[::1] 
test_connection.py::test_pasv_connection_port_reused[127.0.0.1] 
test_connection.py::test_pasv_connection_port_reused[::1] 
test_connection.py::test_pasv_connection_pasv_forced_response_address[127.0.0.1] /Users/denis/code/aioftp/.venv/lib/python3.8/site-packages/coverage/inorout.py:503: CoverageWarning: Module test_connection.py::test_pasv_connection_pasv_forced_response_address was never imported. (module-not-imported)
  self.warn(f"Module {pkg} was never imported.", slug="module-not-imported")
/Users/denis/code/aioftp/.venv/lib/python3.8/site-packages/coverage/control.py:887: CoverageWarning: No data was collected. (no-data-collected)
  self._warn("No data was collected.", slug="no-data-collected")
WARNING: Failed to generate report: No data to report.

!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
========================= 1 failed, 44 passed in 1.67s =========================
PASSED         [  2%]PASSED               [  2%]PASSED                    [  2%]
test_connection.py::test_connection[::1] PASSED                          [  2%]
test_connection.py::test_quit[127.0.0.1] PASSED                          [  2%]PASSED                                [  3%]PASSED               [  3%]PASSED                     [  3%]PASSED                  [  3%]PASSED                        [  3%]PASSED       [  3%]PASSED             [  3%]PASSED         [  3%]PASSED               [  4%]PASSED [  4%]PASSED [  4%]PASSED     [  4%]PASSED     [  4%]PASSED [  4%]PASSED     [  4%]PASSED         [  5%]PASSED               [  5%]PASSED      [  5%]PASSED            [  5%]PASSED   [  5%]PASSED         [  5%]FAILED [  5%]
tests/test_connection.py:130 (test_pasv_connection_pasv_forced_response_address[127.0.0.1])
pair_factory = <class 'conftest.pair_factory.<locals>.Factory'>
Server = <class 'conftest.Container'>, unused_tcp_port = 61376

    @pytest.mark.asyncio
    async def test_pasv_connection_pasv_forced_response_address(
        pair_factory,
        Server,
        unused_tcp_port,
    ):
        def ipv4_used():
            try:
                ipaddress.IPv4Address(pair.host)
                return True
            except ValueError:
                return False

        async with pair_factory(
            server=Server(ipv4_pasv_forced_response_address="127.0.0.2"),
        ) as pair:
            assert pair.server.ipv4_pasv_forced_response_address == "127.0.0.2"

            if ipv4_used():
                # The connection fails here because the server starts to listen for
                # the passive connections on the host (IPv4 address) that is used
                # by the control channel. In reality, if the server is behind NAT,
                # the server is reached with the defined external IPv4 address,
                # i.e. we can check that the connection to
                # pair.server.ipv4_pasv_forced_response_address failed to know that
                # the server returned correct external IP
                with pytest.raises(OSError):
>                   await pair.client.get_passive_connection(commands=["pasv"])

test_connection.py:158: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../src/aioftp/client.py:1160: in get_passive_connection
    reader, writer = await self._open_connection(ip, port)
../.venv/lib/python3.8/site-packages/siosocks/io/asyncio.py:107: in open_connection
    reader, writer = await asyncio.open_connection(host, port, **open_connection_extras)
../../../.pyenv/versions/3.8.18/lib/python3.8/asyncio/streams.py:52: in open_connection
    transport, _ = await loop.create_connection(
../../../.pyenv/versions/3.8.18/lib/python3.8/asyncio/base_events.py:1010: in create_connection
    sock = await self._connect_sock(
../../../.pyenv/versions/3.8.18/lib/python3.8/asyncio/base_events.py:924: in _connect_sock
    await self.sock_connect(sock, address)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <_UnixSelectorEventLoop running=False closed=False debug=False>
sock = <socket.socket [closed] fd=-1, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6>
address = ('127.0.0.2', 61379)

    async def sock_connect(self, sock, address):
        """Connect to a remote socket at address.

        This method is a coroutine.
        """
        _check_ssl_socket(sock)
        if self._debug and sock.gettimeout() != 0:
            raise ValueError("the socket must be non-blocking")

        if not hasattr(socket, 'AF_UNIX') or sock.family != socket.AF_UNIX:
            resolved = await self._ensure_resolved(
                address, family=sock.family, proto=sock.proto, loop=self)
            _, _, _, _, address = resolved[0]

        fut = self.create_future()
        self._sock_connect(fut, sock, address)
>       return await fut
E       asyncio.exceptions.CancelledError

../../../.pyenv/versions/3.8.18/lib/python3.8/asyncio/selector_events.py:496: CancelledError

During handling of the above exception, another exception occurred:

pair_factory = <class 'conftest.pair_factory.<locals>.Factory'>
Server = <class 'conftest.Container'>, unused_tcp_port = 61376

    @pytest.mark.asyncio
    async def test_pasv_connection_pasv_forced_response_address(
        pair_factory,
        Server,
        unused_tcp_port,
    ):
        def ipv4_used():
            try:
                ipaddress.IPv4Address(pair.host)
                return True
            except ValueError:
                return False

        async with pair_factory(
            server=Server(ipv4_pasv_forced_response_address="127.0.0.2"),
        ) as pair:
            assert pair.server.ipv4_pasv_forced_response_address == "127.0.0.2"

            if ipv4_used():
                # The connection fails here because the server starts to listen for
                # the passive connections on the host (IPv4 address) that is used
                # by the control channel. In reality, if the server is behind NAT,
                # the server is reached with the defined external IPv4 address,
                # i.e. we can check that the connection to
                # pair.server.ipv4_pasv_forced_response_address failed to know that
                # the server returned correct external IP
                with pytest.raises(OSError):
>                   await pair.client.get_passive_connection(commands=["pasv"])

test_connection.py:158: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
conftest.py:140: in __aexit__
    await self.timeout.__aexit__(*exc_info)
../.venv/lib/python3.8/site-packages/async_timeout/__init__.py:141: in __aexit__
    self._do_exit(exc_type)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <async_timeout.Timeout object at 0x1115ab540>
exc_type = <class 'asyncio.exceptions.CancelledError'>

    def _do_exit(self, exc_type: Optional[Type[BaseException]]) -> None:
        if exc_type is asyncio.CancelledError and self._state == _State.TIMEOUT:
            assert self._task is not None
            _uncancel_task(self._task)
            self._timeout_handler = None
            self._task = None
>           raise asyncio.TimeoutError
E           asyncio.exceptions.TimeoutError

../.venv/lib/python3.8/site-packages/async_timeout/__init__.py:228: TimeoutError
/Users/denis/code/aioftp/.venv/lib/python3.8/site-packages/pytest_cov/plugin.py:298: CovReportWarning: Failed to generate report: No data to report.

  self.cov_controller.finish()

Process finished with exit code 1

[pohmelie]

Maybe you have something on 127.0.0.2? Try some non-existent ip address instead 127.0.0.2 in tests.

[9en9i]

Tried a different set of addresses, nothing worked.

[pohmelie]

Try test this inside docker container. Maybe it is MacOS relevant problem?

[pohmelie]

Or, maybe you have pushed code somewhere so I can take a look why it is stuck.

[9en9i]

It could be macOS. I'll run it in docker tomorrow first.

[9en9i]

And I checked running tests from the main branch, I get the same timeout error.

[pohmelie]

Here is the code to reproduce expected behavior:

import asyncio
import time

async def main():
    try:
        start = time.perf_counter()
        await asyncio.open_connection("127.0.0.2", 2121)
    except OSError as e:
        print("open_connection exception", e)
    finally:
        print("time elapsed", time.perf_counter() - start)

asyncio.run(main())

I have such output on ubuntu 22.04 and python 3.11.2:

open_connection exception [Errno 111] Connect call failed ('127.0.0.2', 2121)
time elapsed 0.00034436199348419905

[9en9i]

I have:

open_connection exception [Errno 60] Connect call failed ('127.0.0.2', 2121)
time elapsed 75.001879666

[pohmelie]

@9en9i try to use non-existent hostname instead of 127.0.0.2

[9en9i]

If I change the IP to localhost, I get this output:

open_connection exception Multiple exceptions: [Errno 61] Connect call failed ('::1', 2121, 0, 0), [Errno 61] Connect call failed ('127.0.0.1', 2121)
time elapsed 0.006213457999999998

[9en9i]

For any non-existent I get the output:

open_connection exception [Errno 8] nodename nor servname provided, or not known
time elapsed 0.144492083

[pohmelie]

Ok, so I need a fix for tests from my side. I will note in pr when it landed.