net.dns.resolve() has unexpected callbacks

[kit-klein]

Expected behavior

net.dns.resolve() should return an ip address in it's callback for the ip arg

Actual behavior

ip always seems to be nil

Test code

Start with a nodemcu based device connected to the internet. Execute the following from the console.

> net.dns.setdnsserver("8.8.8.8", 0)
> 
> print(net.dns.getdnsserver(0))
8.8.8.8
> 
> 
> net.dns.resolve("google.com", function(sk, ip) if (ip == nil) then print("DNS fail!") else print(ip) end end)
> DNS fail!

> 
> net.dns.resolve("google.com", function(sk, ip) if (sk == nil) then print("DNS fail!") else print(sk) end end)
> 142.250.64.110

You can see in the first call of net.dns.resolve we get nil for the expected ip (which is the second argument in the docs). It appears the ip is actually placed in the first cb argument based on the next attempt.

NodeMCU startup banner

NodeMCU ESP32 built with Docker provided by frightanic.com
        branch: dev-esp32
        commit: 7c07f914836d73e22e862c149def8b3314b77e43
        SSL: true
        modules: -
 build created on 2021-01-04 16:25
 powered by Lua 5.1.4 on ESP-IDF v3.3.2 on SDK IDF

Hardware

Konnected Pro board but should apply to any esp32 based device running nodemcu.

[nwf]

Interesting... can you cross-validate with an ESP8266? I wonder if this is some, er, evolutionary history and we should just stamp it all out to having one argument to net.dns.resolve()'s callback.

[kit-klein]

@nwf it looks like the 8266 does populate the first arg. Although it's always nil...

https://github.com/nodemcu/nodemcu-firmware/blob/release/app/modules/net.c#L872

[nwf]

We should probably reconcile that. Since pushing nil is just a silly thing done for back-compat w/ the per-socket DNS resolver paths, I assume, I'd propose eliminating it and updating both branches and their docs to just pass the one argument to the callback.

[HHHartmann]

I agree but this is a breaking change which silently breaks things. We should add our deprecation message for one release before changing it. Maybe we need to silence the deprecation message a bit since this could lead to problems when many resolutions are performed and each time the dep message is printed.
Maybe after some number of printed messages (say 50) only print it on every nth invocation.

[nwf]

We can...

• push the result twice to the stack (so that it's both the first and second argument) and emit a deprecation warning for the next release.
• oxt release, just push the argument once.

I think I don't mind printing every invocation; our network stack is not particularly high performance, so surely nobody's doing that many, tightly.

[HHHartmann]

That sounds reasonable.

[marcelstoer]

push the result twice to the stack (so that it's both the first and second argument) and emit a deprecation warning for the next release.

👍 I wonder what the initial idea was with having the socket in the callback. To chain calls right after DNS resolution?

[ilkka]

Just lost a good couple hours tracking down "why DNS doesn't work" when I should have known to look in the "always nil" first callback argument 🙄