Various std net improvements

[elcritch]

Expanding the std/net API for a few embedded use cases where calling getAddrInfo repeatedly becomes expensive (and DNS often isn't even enabled). However, it's also nice to be able to send a UDP packet directly to an IpAddress in regular environments when you know the IP upfront (e.g. multicast). Also included is a couple of tweaks to inet_ntop and inet_pton to extend their types, but in a backwards compatible way.

One alteration to the extended API's could be to use an openArray of IpAddress's to allow binding to the first workable IP. Though that seems easy enough to implement oneself.

I'll be testing these manually a bit more, but have included tests and tried them out briefly and they don't error out.

[elcritch]

I'm not sure I like how many new proc this approach requires. However, I get failures on embedded devices from sendTo and recvFrom using the current procs since the getAddrInfo fails. I've had that on both ESP/FreeRTOS and Zephyr.

@Araq would optional arguments (say resolveNames=false) be possible / non-breaking?

[Araq]

@Araq would optional arguments (say resolveNames=false) be possible / non-breaking?

Yes, but maybe the better idea here is an embedded_networking module/package? What would the overlap be with the existing net.nim? How much of it is actually usable as it is?

[elcritch]

Yes, but maybe the better idea here is an embedded_networking module/package? What would the overlap be with the existing net.nim?

It's crossed my mind a few times recently. There's a few other little structural changes that might be nice to make but aren't possible with std/net.

Though, I was thinking of calling it something like litenet.nim or something. There's cases where regular x86 code might prefer not calling into the getAddrInfo and the name lookup overhead. Perhaps games using UDP packets, etc. On regular PCs it's pretty small overhead, but still overhead.

Could something like that live in std or fushion?

How much of it is actually usable as it is?

Actually quite a bit of it is usable. Especially with TCP, but I've not really tried SSL but I suspect it might work with fiddling.

UDP seems a little trickier because of both the extra overhead of parsing the host string on every call. Using recvFrom results in an OSError because it calls getAddrString which seems to try to do a DNS lookup. I think ESP/FreeRTOS had a similar issue. You can use inet_pton instead which is what I set nativenet/accept to do.

[elcritch]

Really for now, it would probably be sufficient to just add recvFrom / sendTo variants with an IpAddress variant as well. Or perhaps a nativenet version that takes SockAddr/SockLen but still does all the nice string buffer handling.

[dom96]

While reviewing this I got a thought: couldn't the recvFrom/connect procs instead detect that the string is an IP address and skip the domain resolution? if this is trivial then we should do it this way.

I think if we do this we can still allow IpAddress to be passed, but maybe this can be done with a generic: proc recvForm[T: IpAddress | string](..., address: var T, ...) to reduce code duplication?

[dom96]

Suggested change

	let bytes = socket.sendTo("192.168.0.1", Port(27960), "status\n")
	echo("sent: ", bytes)
	doAssert socket.sendTo("192.168.0.1", Port(27960), "status\c\l") == 8

[elcritch]

While reviewing this I got a thought: couldn't the recvFrom/connect procs instead detect that the string is an IP address and skip the domain resolution? if this is trivial then we should do it this way.

It crossed my mind too, but for IPv4 it can be a bit difficult to tell if it's an IP address or just a weird domain.

I think if we do this we can still allow IpAddress to be passed, but maybe this can be done with a generic: proc recvForm[T: IpAddress | string](..., address: var T, ...) to reduce code duplication?

This was more of my first approach, and I think it's likely better. It's tricky for a couple of the proc's but doable. I wasn't sure if the generic was frowned up in the standard library. The help docs were easier then to say "will parse hostname, or if an IpAddress will use it directly".

[dom96]

btw why does ESP/FreeRTOS have issues with getAddrInfo for you? I don't recall having trouble resolving domains on my ESP32.

[elcritch]

btw why does ESP/FreeRTOS have issues with getAddrInfo for you? I don't recall having trouble resolving domains on my ESP32.

It depends partly on the configuration. I generally disable DNS service. The esp/freertos may have been fine with getAddrInfo itself (where zephyr returns an os error presumably since I have dns off).

On the esp/freertos I got errors with UDP sendTo/recvFrom, but only after 20-30 minutes of runtime. It seemed to be due to malloc/free's that occur with getAddrInfo. Calling it once and caching and manually calling the posix sendto/recvfrom's resolved the issue. I got lots of vague and hard to track down issues with esp-lib's malloc.

[Araq]

Nit: just use one arg separator style

Actually I prefer the semicolon variant, it's slightly easier to read.

[dom96]

Actually I prefer the semicolon variant, it's slightly easier to read.

I don't really like it. It only ever makes sense for a very specific case: when you have many generic parameters. In this case you don't have them so really don't see how it makes anything more readable.

[elcritch]

Ok I incorporated the above changes. I removed the changes to connect and acceptAddr after figuring out that you can skip DNS resolution using AI_NUMERICHOST in the flags. For TCP connections this seems sufficient.

That leaves recvFrom / sendTo as the major changes. It's still useful to have proc's that don't call getAddrInfo at all since it's overhead on every UDP packet. It also leads to memory corruption issues on some versions of esp-idf.

I changed recvFrom to use template version and used the comma arg separators.

[dom96]

I think this should throw instead if the requested size is too large. Or maybe even better just call setLen on data instead.

[elcritch]

Actually, I should just remove the size: int argument, it's not on the other sendTo string variant. Likely I got it mixed in by copying from the raw pointer sendTo.

[Araq]

The test failure seems relevant?

tests\stdlib\tnet_ll.nim
2021-11-24T06:25:12.2812025Z
2021-11-24T06:25:12.2812650Z [Suite] inet_ntop tests
2021-11-24T06:25:12.2816441Z D:\a\1\s\tests\stdlib\tnet_ll.nim(30, 15): Check failed: res == "19.18.17.16"
2021-11-24T06:25:12.2818516Z res was 19.0.0.0
2021-11-24T06:25:12.2819420Z [FAILED] IP V4

[elcritch]

tests\stdlib\tnet_ll.nim

Yes, I thought I'd fixed it but it's failing somewhere. Its dealing with hard-coded IP addresses, so I should be able to fix it up. It might make sense to add a test for the Port endianness in the same location.

[elcritch]

@Araq @dom96 it looks like there's an issue with Azure pipelines. It seems to be affecting other PR's as well. Is there something we can do to get them running again?

[Araq]

The CIs are broken, we know, MS is working on it, there is nothing we can do, unfortunately. PRs will be merged after the CIs got fixed.

[ringabout]

The CIs are broken, we know, MS is working on it, there is nothing we can do, unfortunately. PRs will be merged after the CIs got fixed.

Is it a known issue tracked somewhere (a link will be appreciated)? Maybe we can try merge some harmless tiny PRs to see whether it helps.

[Araq]

https://stackoverflow.com/questions/68033977/errorno-hosted-parallelism-has-been-purchased-or-granted

[PMunch]

https://stackoverflow.com/questions/68033977/errorno-hosted-parallelism-has-been-purchased-or-granted

According to that this is a policy change and we need to submit a request? Or am I missing something?

[ringabout]

According to that this is a policy change and we need to submit a request? Or am I missing something?

It seems to work now, you can try to close/reopen your PR or restart azure pipelines for your PR according to the contributing guide.

[narimiran]

we need to submit a request?

I did exactly that couple of days ago, and today they replied and gave us (back) parallel builds.

[dom96]

tslow_tables.nim timed out. Re-running.

[elcritch]

tslow_tables.nim timed out. Re-running.

Thanks, it seems there's (still) an issue on some of the WIndows build. It looks like it's only copying the first byte, which doesn't make sense.

[elcritch]

Doing print-debugging since I don't have a Windows dev env setup. Here's a printout of various tests to figure out why InAddr isn't being properly initialized on the Windows amd64_batch2_3.

Nim code:

  test "IP V4":
    # regular
    var ip4 = InAddr(s_addr: 0x10111213'u32)
    # direct assign
    var ip4v2: InAddr
    ip4v2.s_addr = 0x10111213'u32
    # pointer cast
    var ip4v3 = InAddr()
    var ip4v3_ptr = cast[ptr uint32](ip4v3.addr)
    ip4v3_ptr[] = 0x10111213'u32
    # pointer as inaddr
    var ip4v4_num = 0x10111213'u32
    var ip4v4: ptr InAddr = cast[ptr InAddr](ip4v4_num.addr())

    var buff: array[0..255, char]
    let r = inet_ntop(AF_INET, cast[pointer](ip4.s_addr.addr), buff[0].addr, buff.len.int32)
    let res = if r == nil: "" else: $r
    when defined(windows):
      echo("WINDOWS inet_ntop: ip4: " & repr(ip4))
      echo("WINDOWS inet_ntop: ip4.s_addr.sizeof: " & $(ip4.s_addr.sizeof()))
      echo("WINDOWS inet_ntop: ip4:s_addr: " & repr(ip4.s_addr))
      echo("WINDOWS inet_ntop: ip4.s_addr == 0x10111213'u32: " & $(ip4.s_addr == 0x10111213'u32))
      echo("WINDOWS inet_ntop: ip4:ptr: " & repr(cast[ptr array[0..3, uint8]](ip4.s_addr.addr)))
      echo("WINDOWS inet_ntop: ip4v2:s_addr: " & repr(ip4v2.s_addr))
      echo("WINDOWS inet_ntop: ip4v2.s_addr == 0x10111213'u32: " & $(ip4v2.s_addr == 0x10111213'u32))
      echo("WINDOWS inet_ntop: ip4v3:s_addr: " & repr(ip4v3.s_addr))
      echo("WINDOWS inet_ntop: ip4v3.s_addr == 0x10111213'u32: " & $(ip4v3.s_addr == 0x10111213'u32))
      echo("WINDOWS inet_ntop: ip4v4:s_addr: " & repr(ip4v4.s_addr))
      echo("WINDOWS inet_ntop: ip4v4.s_addr == 0x10111213'u32: " & $(ip4v4.s_addr == 0x10111213'u32))
      echo("WINDOWS inet_ntop: buff:len: " & $(buff.len))
      echo("WINDOWS inet_ntop: buff: " & repr(buff))
      echo("WINDOWS inet_ntop: r: " & repr(r))
      echo("WINDOWS inet_ntop: res: " & repr(res))
    check: res == "19.18.17.16"
      

The winlean.nim implementation of InAddr is:

  InAddr* {.importc: "IN_ADDR", header: "winsock2.h".} = object
    s_addr*: uint32  # IP address

2021-12-06T21:15:12.7099947Z [Suite] inet_ntop tests
2021-12-06T21:15:12.7102438Z WINDOWS inet_ntop: ip4: []
2021-12-06T21:15:12.7103475Z WINDOWS inet_ntop: ip4.s_addr.sizeof: 4
2021-12-06T21:15:12.7104703Z WINDOWS inet_ntop: ip4:s_addr: 19
2021-12-06T21:15:12.7105747Z WINDOWS inet_ntop: ip4.s_addr == 0x10111213'u32: false
2021-12-06T21:15:12.7106792Z WINDOWS inet_ntop: ip4:ptr: ptr 00007ff7c47a5504 --> [19, 0, 0, 0]
2021-12-06T21:15:12.7107903Z WINDOWS inet_ntop: ip4v2:s_addr: 269554195
2021-12-06T21:15:12.7108988Z WINDOWS inet_ntop: ip4v2.s_addr == 0x10111213'u32: true
2021-12-06T21:15:12.7110017Z WINDOWS inet_ntop: ip4v3:s_addr: 269554195
2021-12-06T21:15:12.7112551Z WINDOWS inet_ntop: ip4v3.s_addr == 0x10111213'u32: true
2021-12-06T21:15:12.7114015Z WINDOWS inet_ntop: ip4v4:s_addr: 269554195
2021-12-06T21:15:12.7115121Z WINDOWS inet_ntop: ip4v4.s_addr == 0x10111213'u32: true
2021-12-06T21:15:12.7116034Z WINDOWS inet_ntop: buff:len: 256
2021-12-06T21:15:12.7123459Z WINDOWS inet_ntop: buff: ['1', '9', '.', '0', '.', '0', '.', '0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0']
2021-12-06T21:15:12.7131058Z WINDOWS inet_ntop: r: 00007ff7c4868500"19.0.0.0"
2021-12-06T21:15:12.7133665Z WINDOWS inet_ntop: res: 0000023d978de360"19.0.0.0"
2021-12-06T21:15:12.7134748Z     D:\a\1\s\tests\stdlib\tnet_ll.nim(58, 15): Check failed: res == "19.18.17.16"
2021-12-06T21:15:12.7135897Z     res was 19.0.0.0
2021-12-06T21:15:12.7136813Z   [FAILED] IP V4

It appears that doing: ip4v2.s_addr = 0x10111213'u32 works properly but var ip4 = InAddr(s_addr: 0x10111213'u32) does not. Could this be a compiler error? For now I'm just going to try and use the manual assignment to get this PR working.

[elcritch]

Or perhaps more likely reason for the windows issue, should winlean be using {.bycopy.} on the winsock2.h objects?

[Araq]

.bycopy shouldn't make a difference. And the CI is green now, what happened?

[dom96]

And the CI is green now, what happened?

The change to manual assignment fixed it I guess

[elcritch]

And the CI is green now, what happened?

The change to manual assignment fixed it I guess

Yes, that was what fixed it. Not clear why the regular constructor doesn't work, but it matches the debug logging output.

[elcritch]

Filed a bug for the Windows constructor issue here #19244.

Is there anything else to do before this could be merged?

[dom96]

Let's get it in :)