Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

calva/vscode wrongly assumes nrepl started at ::1 (ipv6) #2310

Open
birdspider opened this issue Sep 11, 2023 · 13 comments
Open

calva/vscode wrongly assumes nrepl started at ::1 (ipv6) #2310

birdspider opened this issue Sep 11, 2023 · 13 comments
Labels

Comments

@birdspider
Copy link

birdspider commented Sep 11, 2023

not sure if its my system/system-update but:

description

calva jack-in fails to connect (ECONNREFUSED).

I think because vscode chooses/returns localhost as ipv6 ::1 (maybe in addition to 127.0.0.1).

However, nrepl.cmdline seems not to be started with ipv6 bindings (see below)

calva repl log:

; Starting Jack-in Terminal: pushd [snip]; clojure -Sdeps '{:deps {nrepl/nrepl {:mvn/version,"1.0.0"},cider/cider-nrepl {:mvn/version,"0.37.1"}}}' -M -m nrepl.cmdline --middleware "[cider.nrepl/cider-middleware]" ; popd
; Using host:port localhost:42907 ...
; Hooking up nREPL sessions ...
; nREPL connection failed: Error: connect ECONNREFUSED ::1:42907
; Failed connecting.

I do not know what changed - that makes vscode/calva use ::1 - it's not required for me/my case.

manual setup works

see param -b, with 0.0.0.0 it should bind to any local address, hence connecting when starting like this, works:

clojure -Sdeps '{:deps {nrepl/nrepl {:mvn/version,"1.0.0"},cider/cider-nrepl {:mvn/version,"0.37.1"}}}' -M -m nrepl.cmdline -b "0.0.0.0" --middleware "[cider.nrepl/cider-middleware]" 
@birdspider
Copy link
Author

workaround (force ipv4)

diff --git a/src/nrepl/index.ts b/src/nrepl/index.ts
index 8d685fea..86c253d3 100644
--- a/src/nrepl/index.ts
+++ b/src/nrepl/index.ts
@@ -130,7 +130,7 @@ export class NReplClient {
    */
   static create(opts: { host: string; port: number; onError: (e) => void }) {
     return new Promise<NReplClient>((resolve, reject) => {
-      const socket = net.createConnection(opts, () => {
+      const socket = net.createConnection({ ...opts, family: 4 }, () => {
         const nsId = client.nextId;
         const cloneId = client.nextId;
         const describeId = client.nextId;

@PEZ PEZ added the connect label Sep 11, 2023
@PEZ
Copy link
Collaborator

PEZ commented Sep 11, 2023

Maybe what changed is some default socket creation options? But how would those change... Very strange.

@birdspider
Copy link
Author

birdspider commented Sep 11, 2023

Maybe what changed is some default socket creation options? But how would those change... Very strange.

since localhost means both ::1 and 127.0.0.1 and nrepl start-server doc states

:bind — bind address, by default "127.0.0.1"

(with the explicit default set here)

I think this is simply under-specified. If calva wants to use nrepl defaults - 127.0.0.1 (ip4) it is, otherwise a specific bind address is needed -b. But thats for the calva devs to decide, or upstream/nrepl to change.

@PEZ
Copy link
Collaborator

PEZ commented Sep 17, 2023

Do you mean that if we connect to the nrepl server on port 127.0.0.1, we would avoid the ambiguity?

A problem is that the nrepl server reports that it starts the nrepl on localhost, and this is what Calva uses to figure out what address to connect to. The nrepl code for this is here, and I can't quite follow: https://github.com/nrepl/nrepl/blob/5839006c5f522fd5e6cf2adbcc5b59e4bd0677dd/src/clojure/nrepl/cmdline.clj#L446

Maybe you should report this on nrepl, @birdspider? I can also do it, but you seem to understand the issue better than I do. Meanwhile I can make a translation localhost -> 127.0.0.1 in Calva when acting on the server started message. You think that makes sense?

@PEZ
Copy link
Collaborator

PEZ commented Sep 17, 2023

Or, maybe that would still be a problem for when we are connecting to some other hostname? Is your workaround, forcing ipv4 better? I'm leaning towards that here.

Please advice. 😄

PEZ added a commit that referenced this issue Sep 17, 2023
PEZ added a commit that referenced this issue Sep 17, 2023
PEZ added a commit that referenced this issue Sep 18, 2023
Poor (dumb) man's way to wait for `close` messages to have been sent
before destroying the socket.

* Fixes #2310
@birdspider
Copy link
Author

birdspider commented Sep 18, 2023

Hi,

@PEZ , sorry I didn't catch your replies, also: I'm not in a hurry with this issue.

the way I see it, the problem is that localhost normally means 1 and/or 2 things, that is 127.0.0.1 or ::1 or both.

Meanwhile I can make a translation localhost -> 127.0.0.1 in Calva when acting on the server started message. You think that makes sense?

I think this would make sense currently, because nrepl (as started by calva, with default args) only ever binds to 127.0.0.1.

I also think that the server-started message is part of the problem (see below).

problem is that the nrepl server reports

well, it might report anything, but with no extra args it hard-binds to 127.0.0.1 (not ::1)


failure scenario:

  1. calva starts a nrepl (with no/default args)
  2. an nrepl is started on "localhost:42907" (actually bound to 127.0.0.1:42907)
  3. then calva reads (server-started-message), presumably parses "localhost". (*)
  4. calva asks vscode/node/OS: please resolve localhost for me: (getent hosts localhost) --> ::1.
    (I seems this is per design/default behaviour, ip6 is preferred for localhost when exists)
  5. now calva/vscode tries to connect to ::1:42907 - but nothing was started on ::1

(*) I didn't check the msg


I only see 3 solutions:

  • calva always defaults to nrepl defaults (127.0.0.1)
  • calva explicitly provides an addr to start (nrepl -b) which it then connects to
  • nrepls start-message should list the bound addr (which calva then uses)

since in this case, calva starts the nrepl - I think the 2nd solution is the appropriate one


PS: another workaround:

  "customJackInCommandLine": "clojure -Sdeps '{:deps {nrepl/nrepl {:mvn/version,\"1.0.0\"},cider/cider-nrepl {:mvn/version,\"0.28.5\"}}}' -M -m nrepl.cmdline -b ::1 --middleware \"[cider.nrepl/cider-middleware]\"",

@PEZ
Copy link
Collaborator

PEZ commented Sep 18, 2023

Thanks!

With these commits, Calva would consistently use 127.0.0.1 instead of localhost, even while the nrepl server reports localhost: 73f172f...2310-ipv6-ambiguity

However, if the nrepl server is started on some host, the connect string could be my-app-being-developed:12345 and currently Calva doesn't do any DNS resolve or anything like that. So I'm thinking that might fail in the same way as localhost does? Or does the link you now provided tell me that this is specific to localhost?

Your first workaround above #2310 (comment) seems to dodge all this and just force ipv4, which looks somewhat like what we want?

@birdspider
Copy link
Author

With these commits ...

I think this is appropriate default behavior since it guarantees compatibility with the auto-started nrepl. A custom connection is custom anyway - with user provided host.

However, if the nrepl server is started on some host, the connect string could be my-app-being-developed:12345 and currently Calva doesn't do any DNS resolve or anything like that. So I'm thinking that might fail in the same way as localhost does?

true, but to my knowledge, remote apps, that support (as in resolved DNS) both ipv4 and ipv6 are usually listening on both (see i.e. apache config)

in such a case

a) it's either remote, probably resolved by dns (by the OS) (which practice could also return N addrs, some ip4, some ip6)
b) it's local, so someone (the developer) put it in his /etc/hosts hopefully only 1 time (or bound the nrepl to both addresses - if thats something nrepl can do)

Or does the link you now provided tell me that this is specific to localhost?

it (getent, dns resolve) seems to always prefer ipv6 - if enabled/configured

getent hosts google.com
2a00:1450:4001:829::200e google.com

Your first workaround above #2310 (comment) seems to dodge all this and just force ipv4, which looks somewhat like what we want?

I think this would break all (incl. remote) ipv6-only host connections


Hm, thinking about the problem a bit more, maybe nrepl should resolve "localhost" and use that address for binding.

@birdspider
Copy link
Author

birdspider commented Sep 18, 2023

maybe nrepl should resolve "localhost" and use that address for binding.

maybe they do that, but "their" jvm returns ipv4:

it can depend on how java is started (-Djava.net.preferIPv6Addresses=true)

~ $ clojure -J-Djava.net.preferIPv6Addresses=true -A:rebel
nREPL server started on port 36905 on host localhost - nrepl://localhost:36905
user=> (java.net.InetSocketAddress. "" 3333)
#object[java.net.InetSocketAddress 0x13390a96 "localhost/[0:0:0:0:0:0:0:1]:3333"]
user=> (java.net.InetSocketAddress. "localhost" 3333)
#object[java.net.InetSocketAddress 0x5ee581db "localhost/[0:0:0:0:0:0:0:1]:3333"]

@birdspider
Copy link
Author

birdspider commented Sep 19, 2023

if I do what calva does, in pure node-js (with an open nrepl)

const net = await import("net");
const b = net.createConnection({host:'localhost',port:36227});

there is no error, even though calva can't connect to the repl.

if I use a nonexisting repl port, for illustration, node/net even errors on both IPs:

[errors]: [
    Error: connect ECONNREFUSED ::1:36220
        at createConnectionError (node:net:1634:14)
        at afterConnectMultiple (node:net:1664:40)
        at TCPConnectWrap.callbackTrampoline (node:internal/async_hooks:130:17) {
      errno: -111,
      code: 'ECONNREFUSED',
      syscall: 'connect',
      address: '::1',
      port: 36220
    },
    Error: connect ECONNREFUSED 127.0.0.1:36220
        at createConnectionError (node:net:1634:14)
        at afterConnectMultiple (node:net:1664:40)
        at TCPConnectWrap.callbackTrampoline (node:internal/async_hooks:130:17) {
      errno: -111,
      code: 'ECONNREFUSED',
      syscall: 'connect',
      address: '127.0.0.1',
      port: 36220
    }
  ]

EDIT: native nodejs cmd nodejs=20.6.1, vscode nodejs=18.15.0

@birdspider
Copy link
Author

I think I found the "reason" this happend at all:

as per vscode-versions there was a switch inside vscode bringing nodejs from 16.17.1 to 18.15.0 in Aug 2023.

Further, as per nodejs/node#39987 (and as explained in nodejs/node#40537 (comment)) node 17 resolves/returns addresses as delivered by resolver/OS - whereas previously (node<17) it ordered them ip4 first.

@birdspider
Copy link
Author

birdspider commented Sep 19, 2023

possible fix in nrepl, returning the actual address

diff --git a/src/clojure/nrepl/socket.clj b/src/clojure/nrepl/socket.clj
--- src/clojure/nrepl/socket.clj
+++ src/clojure/nrepl/socket.clj
@@ -207,9 +207,9 @@
           (URI. (str transport-scheme
                      (when (instance? SSLServerSocket sock)
                        "s"))
                 nil ;; userInfo
-                (-> sock .getInetAddress .getHostName)
+                (-> sock .getInetAddress .getHostAddress)
                 (.getLocalPort sock)
                 nil       ;; path
                 nil       ;; query
                 nil)))))) ;; fragment

maybe @bbatsov has some ideas if this is to handle in nrepl or calva

@PEZ
Copy link
Collaborator

PEZ commented Sep 20, 2023

Thanks for doing all this research! Please consider reporting it on the nrepl repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants