Controller should better check listen argument

[ares]

When I run controller like this (note 0.0.0.0/0)

receptor --debug --node-id controller -d /tmp/controller controller --listen=receptor://0.0.0.0/0:8888

here's the output

[root@foreman-nuc1 system]# receptor --debug --node-id controller -d /tmp/controller controller --listen=receptor://0.0.0.0/0:8888
INFO 2020-01-15 09:43:13,528 controller entrypoints Running as Receptor controller with ID: controller
INFO 2020-01-15 09:43:13,529 controller controller Serving on receptor://0.0.0.0/0:8888

not reporting any issue, but in fact it does not listen on 8888. It should either hard fails with some non 0 exit code or print error to logs.

[Ichimonji10]

Receptor doesn't require the --node-id or --listen arguments, so an even more concise reproducer is to concurrently run the following:

receptor -d "$(mktemp --directory)" node

The second process to start should emit an error and continue running:

$ receptor -d "$(mktemp --directory)" node
Task exception was never retrieved
future: <Task finished name='Task-1' coro=<start_server() done, defined at /usr/lib/python3.8/asyncio/streams.py:58> exception=OSError(98, "error while attempting to bind on address ('0.0.0.0', 8888): address already in use")>
Traceback (most recent call last):
  File "/usr/lib/python3.8/asyncio/streams.py", line 94, in start_server
    return await loop.create_server(factory, host, port, **kwds)
  File "/usr/lib/python3.8/asyncio/base_events.py", line 1459, in create_server
    raise OSError(err.errno, 'error while attempting '
OSError: [Errno 98] error while attempting to bind on address ('0.0.0.0', 8888): address already in use

[Ichimonji10]

The original report points out that an invalid address like receptor://0.0.0.0/0:8888 is consumed, and receptor continues running but without crashing. It's also worth pointing out that an invalid address like harglebargle://0.0.0.0:8888 is consumed, and receptor crashes with an unhelpful traceback. It's a separate expression of this same issue.

[Ichimonji10]

If the "conflicting ports" issue is ever fixed, an automated test based off this can be implemented:

def test_conflicting_ports():
    """Start two nodes, and tell them to bind to the same address and port.

    One node should correctly start, and the other should exit with a complaint
    about the requested port being unavailable. See `receptor #93`_.

    .. _receptor #93: https://github.com/project-receptor/receptor/issues/93
    """
    listen = f'receptor://127.0.0.1:{utils.random_port()}'
    nodes = (Node(str(uuid4()), listen=listen), Node(str(uuid4()), listen=listen))
    nodes[0].start()
    nodes[1].start()  # receptor emits error and keeps running
    for node in nodes:
        node.stop()

[Ichimonji10]

Some malformed listen URLs are rejected:

$ poetry run receptor --data-dir="$(mktemp --directory)" node --listen 'harglebargle://127.0.0.1:8888'
ERROR 2020-03-05 13:58:53,385  __main__ main: an error occured while running receptor
Traceback (most recent call last):
  File "/home/ichimonji10/code/receptor/receptor/__main__.py", line 59, in main
    config.go()
  File "/home/ichimonji10/code/receptor/receptor/config.py", line 525, in go
    self._parsed_args.func(self)
  File "/home/ichimonji10/code/receptor/receptor/entrypoints.py", line 49, in run_as_node
    listen_tasks = controller.enable_server(config.node_listen)
  File "/home/ichimonji10/code/receptor/receptor/controller.py", line 54, in enable_server
    listener = self.connection_manager.get_listener(url)
  File "/home/ichimonji10/code/receptor/receptor/connection/manager.py", line 48, in get_listener
    raise RuntimeError(f"Unknown URL scheme {service.scheme}")
RuntimeError: Unknown URL scheme harglebargle

The bind address provided in the original bug report is valid, so I don't see any reason to block on it:

>>> from urllib.parse import urlparse
>>> urlparse('receptor://127.0.0.1/0:8888')
ParseResult(scheme='receptor', netloc='127.0.0.1', path='/0:8888', params='', query='', fragment='')

Receptor continues to run even when it's unable to bind to a port:

$ poetry run receptor --data-dir="$(mktemp --directory)" node
$ poetry run receptor --data-dir="$(mktemp --directory)" node
ERROR 2020-03-05 13:57:20,800  controller [Errno 98] error while attempting to bind on address ('0.0.0.0', 8888): address already in use
Traceback (most recent call last):
  File "/home/ichimonji10/code/receptor/receptor/controller.py", line 46, in exit_on_exceptions_in
    await task
  File "/usr/lib/python3.8/asyncio/streams.py", line 94, in start_server
    return await loop.create_server(factory, host, port, **kwds)
  File "/usr/lib/python3.8/asyncio/base_events.py", line 1459, in create_server
    raise OSError(err.errno, 'error while attempting '
OSError: [Errno 98] error while attempting to bind on address ('0.0.0.0', 8888): address already in use

That's a problem, but it's distinct from "does receptor check the syntactic and semantic validity of the listen address it's given," so I'll file a separate issue for that.

[Ichimonji10]

I'd like to close this issue, but I lack the appropriate permissions.

[ghjm]

The bind address provided in the original bug report is valid, so I don't see any reason to block on it

I don't think we really should be taking a URL as a parameter to --listen in the first place. As long as we are, it's hard to know what we should do with a valid URL that isn't a valid thing to listen for.

[Ichimonji10]

You're thinking of making e.g. --listen-port and --listen-address available? That makes more sense to me.

[ghjm]

You would also need a --listen-protocol and as a result, it would be a lot more verbose. Maybe file that as an enhancement issue to look at in the future?

[Ichimonji10]

I've filed the following issues:

• Receptor continues running if unable to bind to listen socket address #152: Receptor continues running if unable to bind to listen socket address
• --listen parameter lets one specify nonsensical behaviour #153: --listen flag lets one specify nonsensical behavior

Closing because the specific issue described by this issue has been resolved, and the questions raised in the course of investigating this issue have been described by these new issues.