irmen / Pyro5

Pyro 5 - Python remote objects
https://pyro5.readthedocs.io
MIT License
303 stars 36 forks source link

starting ns checks differently host than locate ns #65

Closed janluak closed 2 years ago

janluak commented 2 years ago

the functions core.locate_ns and nameserver.start_ns check differently wether host is "localhost" or not:

lines based on latest_commit: f182529

This leads to a different behaviour of starting and looking for nameserver based on your host-system being identified as "localhost" or "127.0.0.1" → if host provides "localhost", locate_ns cannot find the nameserver.

suggested fix: change line 701 in Pyro5.nameserver to if hostip.startswith("127.") or hostip in ("localhost", "::1"):

irmen commented 2 years ago

How is it possible that getsockname() returns "localhost" ? I thought it always returns an IP address.

irmen commented 2 years ago

In any case, I pushed a small change 1ac493342145b7b87bb0754244d5515533612a3d can you verify that this works for you?

janluak commented 2 years ago

Hey, thanks for the quick fix!

So the starting of the broadcast server with "localhost" is fixed. Therefore, the check bcserver is None is now consistent.

Yet, I can't get my head around why locating a name_server on localhost isn't working. I added some tests to get a better understanding of your implementation but still, I cannot figure out why locate_ns isn't working on localhost (even without broadcast server running). I'd expect to have all tests pass since all parameter are always referencing the default config...

Also, why is start_ns(host="") behaving differently than start_ns (host=None)?


class TestNameServerLocalhost:
    @staticmethod
    def _get_ip_of_host():
        import socket
        sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        try:
            sock.connect(('10.255.255.255', 1))
            ip_address = sock.getsockname()[0]
        finally:
            sock.close()
        return ip_address

    def setup_name_server(self, host, broadcast):
        config.POLLTIMEOUT = 0.1
        self.nsUri, self.nameserver, self.bcserver = Pyro5.nameserver.start_ns(host=host, enableBroadcast=broadcast)
        if self.bcserver is not None:
            self.bcthread = self.bcserver.runInThread()

    def teardown_method(self):
        time.sleep(0.01)
        self.nameserver.shutdown()
        if self.bcserver is not None:
            self.bcserver.close()

    @pytest.mark.timeout(2)
    @pytest.mark.parametrize(
        ("host", "port", "broadcast"),
        (
                ("", None, True),   # default config
                ("", None, False),
                ("localhost", None, True),
                ("localhost", None, False),
                ("localhost", 9090, True),
                ("localhost", 9090, False),
                ("127.0.0.1", None, True),
                ("127.0.0.1", None, False),
                ("127.0.0.1", 9090, True),
                ("127.0.0.1", 9090, False),
        )
    )
    @pytest.mark.parametrize(
        "start_ns_param_host",
        (
                "",
                None,
        )
    )
    def test_locate_ns_on_localhost(self, start_ns_param_host, host, port, broadcast):
        self.setup_name_server(start_ns_param_host, broadcast)
        if host:
            assert host in self.nsUri
        if broadcast:
            assert self.bcserver
        else:
            assert self.bcserver is None
        ns = Pyro5.core.locate_ns(host=host, port=port, broadcast=broadcast)  # lookup on localhost
        assert isinstance(ns, Pyro5.client.Proxy)
        ns._pyroRelease()

    def test_locate_ns_with_host_ip(self):
        self.setup_name_server(self._get_ip_of_host(), True)
        assert self.bcserver
        ns = Pyro5.core.locate_ns()  # broadcast lookup even if host ip
        assert isinstance(ns, Pyro5.client.Proxy)
        ns._pyroRelease()
irmen commented 2 years ago

why locate_ns isn't working on localhost

I've never seen this fail so it's some funky setup that causes it. I tried messing with my /etc/hosts yesterday to reroute "localhost" to something else but even that didn't break it.

How is the above test file supposed to work? You're starting a name server daemon in setup_name_server but its event loop is never called so the test blocks immediately as soon as a connection is made in the locate_ns() call because the request is never served.

janluak commented 2 years ago

I am working on a M1 Mac natively and within docker. Something seems to be really off...

Well, I was comparing to your class TestNameServer0000: the name_server isn't going into event loop either, only bcserver is.

Anyways, I added an event loop and in deed: now 3 instead of 2 tests are green.

import threading

class TestNameServerLocalhost:
    _run = True

    def _condition(self):
        return self._run

    @staticmethod
    def _get_ip_of_host():
        import socket
        sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        try:
            sock.connect(('10.255.255.255', 1))
            ip_address = sock.getsockname()[0]
        finally:
            sock.close()
        return ip_address

    def setup_name_server(self, host, broadcast):
        config.POLLTIMEOUT = 0.1
        self.nsUri, self.nameserver, self.bcserver = Pyro5.nameserver.start_ns(host=host, enableBroadcast=broadcast)
        self.thread = threading.Thread(target=self.nameserver.requestLoop, args=[self._condition])
        self.thread.start()
        if self.bcserver is not None:
            self.bcthread = self.bcserver.runInThread()

    def teardown_method(self):
        self._run = False
        time.sleep(0.01)
        self.nameserver.shutdown()
        if self.bcserver is not None:
            self.bcserver.close()
        self.thread.join(0.1)

    @pytest.mark.timeout(2)
    @pytest.mark.parametrize(
        ("host", "port", "broadcast"),
        (
                ("", None, True),   # default config
                ("", None, False),
                ("localhost", None, True),
                ("localhost", None, False),
                ("localhost", 9090, True),
                ("localhost", 9090, False),
                ("127.0.0.1", None, True),
                ("127.0.0.1", None, False),
                ("127.0.0.1", 9090, True),
                ("127.0.0.1", 9090, False),
        )
    )
    @pytest.mark.parametrize(
        "start_ns_param_host",
        (
                "",
                None,
        )
    )
    def test_locate_ns_on_localhost(self, start_ns_param_host, host, port, broadcast):
        self.setup_name_server(start_ns_param_host, broadcast)
        if host:
            assert host in self.nsUri
        if broadcast:
            assert self.bcserver
        else:
            assert self.bcserver is None
        ns = Pyro5.core.locate_ns(host=host, port=port, broadcast=broadcast)  # lookup on localhost
        assert isinstance(ns, Pyro5.client.Proxy)
        ns._pyroRelease()

    def test_locate_ns_with_host_ip(self):
        self.setup_name_server(self._get_ip_of_host(), True)
        assert self.bcserver
        ns = Pyro5.core.locate_ns()  # broadcast lookup even if host ip
        assert isinstance(ns, Pyro5.client.Proxy)
        ns._pyroRelease()
irmen commented 2 years ago

The assert host in self.nsUri line is weird, URI's are not iterable. So when I run the tests, almost all fail with:

      assert host in self.nsUri

E TypeError: argument of type 'URI' is not iterable

irmen commented 2 years ago

@janluak sorry for the ping. but do you think you can clean up that issue with the test, mentioned above?

irmen commented 2 years ago

The difference between host="" and host=None is documented here https://pyro5.readthedocs.io/en/latest/servercode.html#Daemon

I'm closing this issue as it seems stale now. Feel free to reopen if this is still occurring.