Replace 0.0.0.0 with 127.0.0.1 for client connections #7766

rkimball · 2021-03-29T20:15:33Z

~~Also change references to 127.0.0.1 to localhost so that all references are consistent.~~

~~On Windows default 0.0.0.0 is not mapped to 127.0.0.1 as it is on Linux and it should not be. 0.0.0.0 is used for an undefined address, not localhost.~~

~~https://en.wikipedia.org/wiki/0.0.0.0~~

~~While 0.0.0.0 can be the same as 127.0.0.1 on 'servers' it is not true on client computers. This PR should make things work better on Windows, but does not address all Windows networking issues.~~

Any socket used for listening should use 0.0.0.0 as the address
Any socket used for client connections to the local machine should use 127.0.0.1, not 0.0.0.0.
Use 127.0.0.1 instead of localhost since localhost could be IPV4 or IPV4. If a test or user specifically wants IPV6 they can use the address :1

tqchen · 2021-03-29T20:34:46Z

on linux 0.0.0.0 allows external host to connect(as it binds on the external accessible IP), while localhost would prevent external machine to connect, which would be problematic for cases like cpp_rpc(to be expected to connect from remote).

Would be great to figure out the examples in each case.
There is also a difference in terms of IPv6 vs IPv4, and explicitly specifying "localhost" might cause some of the problem

rkimball · 2021-03-29T20:43:06Z

0.0.0.0 is not a valid address on Windows. On linux if you ping 0.0.0.0 it actually ends up pinging 127.0.0.1. I will try an example of using 127.0.0.1 and connecting from an external host.

According to this https://en.wikipedia.org/wiki/Localhost 127.0.0.1 and localhost are the same. If you are more comfortable with 127.0.0.1 then I can change to that.

tqchen · 2021-03-29T21:05:32Z

0.0.0.0 maps to all possible address on linux. While 127.0.0.1 is equivalent as localhost.

Say the IP of the node is 192.168.0.3, then we either need to set the host to be 192.168.0.3 or 0.0.0.0 on linux(which binds to both 127.0.0.1 and 192.168.0.3).

This is useful under a case of dynamic DNS where server can report to tracker but cannot hardcode a IP(the same command would work for multiple cases).

tqchen · 2021-03-29T21:06:44Z

Also 127.0.0.1 can be different from localhost(because of IPv6) and some code assumes IPv6, so 127.0.0.1 can be useful in certain cases(e.g. MacOS), or we need to add IPv6 compact support in most cases

rkimball · 2021-03-29T21:58:07Z

On Windows where I am having issues 0.0.0.0, or really IN_ADDRANY in the sockaddr structure, is needed for a server socket, one that is listening for connections. This address will bind to all local address. This works on windows and I am able to connect from both local and remote machines to a windows tracker bound to 0.0.0.0.
For client connections, any connection to a listening socket, on windows we can't use 0.0.0.0 as the target address. This works on linux but not on windows. So we are going to have a mix of addresses. For servers we must use 0.0.0.0 but clients we can't use that.
For client connections we can use either 127.0.0.1 or localhost for any self-hosted server.

tqchen · 2021-03-29T22:47:26Z

Agree 0.0.0.0 is needed for server listen addr only but not for client connection

tqchen · 2021-03-30T12:41:41Z

@jroesch @mdw-octoml would be great if you can also help provide some thoughts

Will let @jroesch manage the PR

mdw-octoml · 2021-03-30T14:30:54Z

So, I'm a little confused here. It seems that this PR is changing the default both for the address the server binds to as well as the address a client would connect to. Am I getting that right?

My understanding is that on both Windows and Linux, binding a server to 0.0.0.0 is the correct thing to do, since it allows the server to listen on any address / any interface, which is necessary in order to accept external connections. Binding to localhost or 127.0.0.1 would only bind on the loopback interface (on Linux, anyway) only permitting local connection, but I don't think that is the correct default behavior.

The client address should, of course, be specified explicitly, either as 127.0.0.1 (or, equivalently, 'localhost') or explicitly when connecting to a remote address. So changing the client default to localhost seems fine, but I would not change the server default.

rkimball · 2021-03-30T16:09:50Z

@mdw-octoml You are correct. This PR is wrong but there is still a problem with addresses which needs to be fixed. I will revert all of the current changes and submit new changes that correct the issues I am having on windows. I would close this PR and create a new one, but there is good discussion here that I would like to preserve.

Question to you and @tqchen: Currently when we create the tracker we pass in the "host" address of 0.0.0.0 which is fine and the tracker has a property "host" which returns the "host address" of the tracker which is not fine. The tracker likely has multiple addresses, one for each of the network interfaces and returning the "host address" of the tracker is confusing, which of the multiple host addresses would a client connect to? The client is normally told explicitly via a command-line parameter which address to connect to. I would like to remove the "host" property of the tracker. Currently the tracker host property returns 0.0.0.0 which is not the address a client would use to connect to the tracker.

We also need to disambiguate the address of the tracker in the code, which uses host and tracker_host interchangeably. The tracker host should be consistently called tracker_host

mdw-octoml · 2021-03-30T20:13:46Z

I would like to remove the "host" property of the tracker. Currently the tracker host property returns 0.0.0.0 which is not the address a client would use to connect to the tracker.

I agree that should be done, but I don't know the implications of removing that property. It seems to me that clients can only discover the tracker through some out of band mechanism anyway.

We also need to disambiguate the address of the tracker in the code, which uses host and tracker_host interchangeably. The tracker host should be consistently called tracker_host

Agreed.

tqchen · 2021-03-30T20:17:07Z

Thanks @rkimball @mdw-octoml I agree with the points being said.

Our main need so far is to keep the port number of tracker available.This is mainly for the ease of writing tests as a common pattern would be starting a tracker. Then a client can request resources from that port. In terms of host, we could always write tests that connects 127.0.0.1 so perhaps we do not strictly need that field(or client should not think that as a field to connect to)

mdw-octoml

LGTM. A few minor nits to address.

mdw-octoml · 2021-04-14T15:33:50Z

python/tvm/rpc/proxy.py

@@ -512,7 +512,7 @@ def __init__(
                self.port = my_port
                break
            except socket.error as sock_err:
-                if sock_err.errno in [98, 48]:
+                if sock_err.errno in [98, 48, 10098, 10048]:


I would recommend using symbols from the errno library here instead of hardcoding values.
If that does not work, could you at least add a comment here indicating what these error codes are?

98/10098 is errno.EADDRINUSE for linux/windows. This works for both.
48 is errno.ELNRNG on linux but ELNRNG is not defined in windows so we can't use the symbol. The error translates to Link number out of range which I don't understand. I removed 48/10048. If we want we can add the number 48 back but we can't use the symbol.

mdw-octoml · 2021-04-14T15:34:02Z

python/tvm/rpc/server.py

@@ -419,7 +419,7 @@ def __init__(
                    self.port = my_port
                    break
                except socket.error as sock_err:
-                    if sock_err.errno in [98, 48]:
+                    if sock_err.errno in [98, 48, 10098, 10048]:


See comment above on errnos.

See comment for above.

mdw-octoml · 2021-04-14T15:34:16Z

python/tvm/rpc/tracker.py

@@ -399,7 +399,7 @@ def __init__(self, host, port=9190, port_end=9199, silent=False):
                self.port = my_port
                break
            except socket.error as sock_err:
-                if sock_err.errno in [98, 48]:
+                if sock_err.errno in [98, 48, 10098, 10048]:


See comment above on errnos.

see comment for above

mdw-octoml · 2021-04-14T15:35:07Z

src/support/socket.h

@@ -85,7 +85,7 @@ inline std::string GetHostName() {
 * \return result of operation.
 */
 inline bool ValidateIP(std::string ip) {
-  if (ip == "localhost") {
+  if (ip == "127.0.0.1") {


This should not be necessary since "127.0.0.1" should pass the is_ipv4 check below.

This should have been left as it was with "localhost" since it is a special valid address.

mdw-octoml · 2021-04-14T15:35:28Z

src/support/socket.h

@@ -118,7 +118,7 @@ struct SockAddr {
    std::string host = url.substr(2, sep - 3);
    std::string port = url.substr(sep + 1, url.length() - 1);
    ICHECK(ValidateIP(host)) << "Url address is not valid " << url;
-    if (host == "localhost") {
+    if (host == "127.0.0.1") {


This should not be necessary, since it's already that value :-)

This should have been left as it was with "localhost" since it is a special valid address.

mdw-octoml · 2021-04-14T15:36:22Z

tests/python/unittest/test_runtime_rpc.py

@@ -45,7 +45,7 @@
 # run individual functions. Somewhere along the way, the imports are being
 # lost, so the server ends up not registering the functions.
 pytestmark = pytest.mark.skipif(
-    multiprocessing.get_start_method() != "fork",
+    sys.platform.startswith("win") == False and multiprocessing.get_start_method() != "fork",


Could you add a comment here why Windows is being excluded here?

Actually this enables this test for Windows. Windows does not support fork so this test could never run on Windows, but now it can. I added a comment stating this

…127.0.0.1 to localhost so that all references are consistent. 0.0.0.0 is not the same as localhost.

This reverts commit 6723126.

* Rename references to 0.0.0.0 to localhost. Also change references to 127.0.0.1 to localhost so that all references are consistent. 0.0.0.0 is not the same as localhost.

tqchen assigned jroesch Mar 30, 2021

tqchen added the status: need review label Mar 30, 2021

rkimball changed the title ~~Rename references to 0.0.0.0 to localhost.~~ Replace 0.0.0.0 with 127.0.0.1 for client connections Mar 30, 2021

rkimball requested a review from yzhliu as a code owner April 7, 2021 20:12

mdw-octoml approved these changes Apr 14, 2021

View reviewed changes

rkimball added 14 commits April 26, 2021 08:56

Rename references to 0.0.0.0 to localhost. Also change references to …

8745b5b

…127.0.0.1 to localhost so that all references are consistent. 0.0.0.0 is not the same as localhost.

Fix merge error

fb91b40

Fix merge error

9bf88d1

Address host address issues in rpc test

1308a5c

Revert "Address host address issues in rpc test"

e4d37b1

This reverts commit 6723126.

Address host address issues in rpc test

8f6aee5

Revert changes

1c5a0ff

Address address issues

f3f86ad

Rename localhost to 127.0.0.1 for connections

830a278

Fix unit test for windows

4d941b7

Change RPCError a RuntimeError to a TVMError

ec7b822

Use errno symbols that work on linux and windows

1369c9f

Use errno symbols that work on linux and windows

ff7637b

Address review comments

2b03126

rkimball added 8 commits April 26, 2021 09:06

fix lint error

0a4ed6e

Fix merge errors

bed5f6d

Remove redundant register

725009b

Remove redundant register

812159f

Fix broken test

7563b02

Fix CI failure

5bc819b

empty

0804917

empty

e890622

tqchen approved these changes May 1, 2021

View reviewed changes

Merge branch 'main' into bob/localhost

07da6d7

tqchen merged commit f4a680d into apache:main May 2, 2021

tqchen added status: accepted and removed status: need review labels May 2, 2021

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace 0.0.0.0 with 127.0.0.1 for client connections #7766

Replace 0.0.0.0 with 127.0.0.1 for client connections #7766

rkimball commented Mar 29, 2021 •

edited

Loading

tqchen commented Mar 29, 2021 •

edited

Loading

rkimball commented Mar 29, 2021

tqchen commented Mar 29, 2021

tqchen commented Mar 29, 2021

rkimball commented Mar 29, 2021

tqchen commented Mar 29, 2021

tqchen commented Mar 30, 2021

mdw-octoml commented Mar 30, 2021

rkimball commented Mar 30, 2021 •

edited

Loading

mdw-octoml commented Mar 30, 2021

tqchen commented Mar 30, 2021 •

edited

Loading

mdw-octoml left a comment

mdw-octoml Apr 14, 2021

rkimball Apr 14, 2021

mdw-octoml Apr 14, 2021

rkimball Apr 14, 2021

mdw-octoml Apr 14, 2021

rkimball Apr 14, 2021

mdw-octoml Apr 14, 2021

rkimball Apr 14, 2021

mdw-octoml Apr 14, 2021

rkimball Apr 14, 2021

mdw-octoml Apr 14, 2021

rkimball Apr 14, 2021 •

edited

Loading

Replace 0.0.0.0 with 127.0.0.1 for client connections #7766

Replace 0.0.0.0 with 127.0.0.1 for client connections #7766

Conversation

rkimball commented Mar 29, 2021 • edited Loading

tqchen commented Mar 29, 2021 • edited Loading

rkimball commented Mar 29, 2021

tqchen commented Mar 29, 2021

tqchen commented Mar 29, 2021

rkimball commented Mar 29, 2021

tqchen commented Mar 29, 2021

tqchen commented Mar 30, 2021

mdw-octoml commented Mar 30, 2021

rkimball commented Mar 30, 2021 • edited Loading

mdw-octoml commented Mar 30, 2021

tqchen commented Mar 30, 2021 • edited Loading

mdw-octoml left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkimball Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

rkimball commented Mar 29, 2021 •

edited

Loading

tqchen commented Mar 29, 2021 •

edited

Loading

rkimball commented Mar 30, 2021 •

edited

Loading

tqchen commented Mar 30, 2021 •

edited

Loading

rkimball Apr 14, 2021 •

edited

Loading