Kylegoetz udp

[kylegoetz]

Overview

There were no UDP types or terms. Now there are:

• UDPSocket
• ListenSocket
• ClientSockAddr
• IO.UDP.clientSocket.impl.v1 : Text -> Text -> Either Failure UDPSocket -- first param is hostname, second is port to connect to
• IO.UDP.ClientSockAddr.toText.v1 :: ClientSockAddr -> Text
• IO.UDP.UDPSocket.toText.impl.v1 :: UDPSocket -> Text
• IO.UDP.UDPSocket.close.impl.v1 :: UDPSocket -> Either Failure ()
• ~~IO.UDP.UDPSocket.socket.impl.v1 :: UDPSocket -> Socket~~ removed, see discussion below
• IO.UDP.serverSocket.impl.v1 :: Text -> Text -> Either Failure ListenSocket -- first param is the IP address, second is port
• IO.UDP.ListenSocket.recvFrom.impl.v1 :: ListenSocket -> Either Failure (Bytes, ClientSockAddr)
• IO.UDP.ListenSocket.sendTo.impl.v1 :: ListenSocket -> Bytes -> ClientSockAddr -> Either Failure ()
• IO.UDP.ListenSocket.toText.impl.v1 :: ListenSocket -> Text
• IO.UDP.ListenSocket.close.impl.v1 :: ListenSocket -> Either Failure ()
• ~~IO.UDP.ListenSocket.socket.impl.v1 :: ListenSocket -> Socket~~ removed, see discussion below
• IO.UDP.UDPSocket.recv.impl.v1 :: UDPSocket -> Either Failure Bytes
• IO.UDP.UDPSocket.send.impl.v1 :: UDPSocket -> Bytes -> Either Failure ()

Closes #4757

Implementation notes

1. Adds Network.UDP (Haskell) and racket/udp (JIT) as dependencies.
2. exposes the above functions as builtins (with a small bit of mapping for Text-wrapping newtypes + Bytes/ByteString mapping)

Interesting/controversial decisions

I chose to use the existing Network.UDP Haskell library because it appeared to be the easiest way to add UDP built-ins to Unison.

The Haskell library tries to parallel a TCP connection-based library even though UDP is a connection-less protocol.

It might have been better to write more Haskell to create our own sockets configurable with UDP. Maybe a Unison Socket builtin that takes the same parameters as the Haskell Socket rather than a pre-baked TCP socket. Then implement everything at a lower-level and expose that to Unison, where Unison could have something like:

type Socket = <builtin>
type UDPSocket = UDPSocket Socket
type TCPSocket = TCPSocket Socket

createUdpClient :: HostName -> Port -> Either Failure UDPSocket
createUdpClient = cases HostName h, Port p -> builtInCreateSocket h p "DATAGRAM"

createTcpClient :: HostName -> Port -> Either Failure TCPSocket
createTcpClient = cases HostName h, Port p -> builtInCreateSocket h p "STREAM"

This might actually be a good improvement in the future because it's my understanding that you might need alternative configuration for the underlying Socket in order to support UDP-based multicast, e.g. Don't quote me on that.

Edit The JIT code mirrors the Haskell, which turns out to be similar under the hood.

Test coverage

There are tests found at @unison/runtime-tests/@kylegoetz/udp

Loose ends

Maybe could use more testing to verify the Racket/JIT is actually spitting out () when it's supposed to (on send-y functions and close).

One important thing to note is that when creating a client, the parameters are hostname and port, but when creating a server, the parameters are IP address and port. Because there is no IPAddress type in base currently, (contrast with HostName), I've relied on readMaybe to cast the Text coming from Unison to Maybe IP and then failing on Nothing but invoking the Haskell server creation on Just ip.

[aryairani]

Hi @kylegoetz this is exciting! You could try try taking a look at tcp-tests.u and see if that provides some inspiration for testing the UDP functionality. The more we can test, the better. Especially with a [email protected] 😅

@runarorama Could you take a look when you have a chance and weigh in on Kyle's Decisions / Tests / and IPAddress loose end? Is there anything you'd change about the API before we locked in some builtins?

@ChrisPenner @dolio Could you give a spot check as well?

[dolio]

Looks okay to me. I was a little skeptical about network-udp-0.0.0, but it looks pretty simple, mostly wrapping network and other stuff, and I don't see any other udp implementations that just provide normal socket APIs (like, not some kind of streaming framework).

[kylegoetz]

@aryairani I've pushed a udp-tests.u and updated the tests.u to reference it.

A couple notes:

1. putting the UDP tests into a scratch file alongside the Tests ability + supporting terms, I was able to main = do Tests.run udp.tests and all the tests pass (and when I intentionally broke some to make sure the tests were written correctly, those tests would fail)
2. putting the code into udp-tests.u. and then updating tests.u with a line !udp.tests below !tcp.tests causes the integration tests to fail.

```ucm
    .> load unison-src/builtin-tests/tests.u.> add
    ```
    🛑
    The transcript failed due to an error in the stanza above. The error is:
      I couldn't figure out what udp.tests refers to here:
         11 |   !udp.tests
      I think its type should be:
          -- [snip]      
          Unit -> Unit

If I copy the udp.tests term into tests.u so there can be no doubt that it will find the tests, then I get the error

I couldn't find a term for ##IO.UDP.clientSocket.impl.v1.
         69 | UDP.client h p = Either.toException (##IO.UDP.clientSocket.impl.v1 h p) 
      Make sure it's spelled correctly.

But in UCM I can view ##IO.UDP.clientSocket.impl.v1 and see

test/main> view ##IO.UDP.clientSocket.impl.v1
  builtin UDP.client.impl :
    lib.base_2_17_0.Text
    -> lib.base_2_17_0.Text
    ->{lib.base_2_17_0.IO} Either Failure UDP.UDPSocket

I don't know if the integration tests uses a pre-built UCM binary rather than my custom built one with the new UDP built-ins using stack build and then stack exec unison, but that would explain the failure. And obviously I'm using the hashes for these built-ins since they're not part of base yet. I've created utility functions for the integration tests

UDP.client h p = Either.toException (##IO.UDP.clientSocket.impl.v1 h p)
UDP.server i p = Either.toException (##IO.UDP.serverSocket.impl.v1 i p)
closeClient = ##IO.UDP.UDPSocket.close.impl.v1 >> Either.toException
send s b = Either.toException (##IO.UDP.UDPSocket.send.impl.v1 s b)
closeServer = ##IO.UDP.ListenSocket.close.impl.v1 >> Either.toException
sendTo s b a = Either.toException (##IO.UDP.ListenSocket.sendTo.impl.v1 s b a)

and Unison doesn't complain about these hashes when part of a scratch file and run as part of the main I pasted above.

[kylegoetz]

@runarorama if IpAddress as a data type that doesn't exist in base currently will be addressed, the underlying Haskell for the IP type used here is (as one would expect) a sum type of IPv4 and IPv6 data constructors. So both v4 and v6 (but no other formats—if there are any) are supported by the UDP library when constructing a UDP "server."

Also, for all involved, if y'all want the interface to be different in some way, I can rewrite or add built-ins. I just tried to parallel the TCP terms/types as closely as possible while taking into account the different natures of UDP vs TCP (i.e., connection-less vs connection-present).

[aryairani]

I merged trunk so that CI will start running again.

[aryairani]

I haven't had a chance to try to reproduce the testing issue you mentioned, but I will try soon.

[aryairani]

I don't know if the integration tests uses a pre-built UCM binary rather than my custom built one with the new UDP built-ins using stack build and then stack exec unison

No, it shouldn't be; if it is, then that's a bug in the CI setup. I think there might be one, I'm looking into it now.

[aryairani]

@kylegoetz I got as far as:

.> load unison-src/builtin-tests/udp-tests.u

  Loading changes detected in unison-src/builtin-tests/udp-tests.u.


  I couldn't figure out what recvFrom refers to here:
  
     48 |         (data, sockAddr) = recvFrom ssocket
  
  I think its type should be:
  
      ListenSocket -> (Bytes, ClientSockAddr)

I can't find recvFrom anywhere; maybe it could be defined locally in udp-tests.u and then eventually added to @unison/base?

[kylegoetz]

@aryairani

I can't find recvFrom anywhere; maybe it could be defined locally in udp-tests.u and then eventually added to @unison/base?

Fixed. I had neglected to add it with the block of other local term aliases.

I've updated the tests. I added a ' after each UDP method called and then re-ran.

The issue was that in my test env I had added various term aliases to my project and one of them was recvFrom. So the fact I hadn't created a recvFrom local to the UDP test file didn't trigger an error. By adding the tick marks (which I know have no terms with those names in my local test project) and re-running the tests, I don't think this could happen again.

All the necessary functions should be there as term aliases for the built-in hash versions.

Edit That is, aliases but wrapped by toException

[aryairani]

Oops, I had forgotten to push a fix for tests, merging now...

[aryairani]

Sorry @kylegoetz I am realizing that some of these testing setups are more convoluted than what we meant to subject contributors (or ourselves) to... I'm looking into some cleanup.

[aryairani]

@kylegoetz Okay, I think I've gotten the CI issues squared away. (I may be saying this prematurely as the latest CI run hasn't finished yet, fingers crossed.) I expect it will pass all portions except for UDP tests on the JIT, obviously.

The next question is do you want to try to tackle the JIT portion of UDP support, or not particularly?

Some other options include:

• we wait for someone else to pick it up
• we just say there's no support for UDP in the JIT for now; as there are a few obstacles still remaining to having a nice end-user distribution for the JIT, this may not be a hugely noticeable problem 😓

[kylegoetz]

@aryairani I will take a look. I know racket has a UDP library so I can look at the TCP implementation in the JIT and see if I can do it.

[aryairani]

@kylegoetz Oh side note, while we are putting finishing touches on collaboration support on Share, if you need to make changes to the UDP tests, you can do it by cloning @unison/runtime-tests/@aryairani/udp and changing this line https://github.com/unisonweb/unison/pull/4844/files#diff-944291df2c9c06359d37cc8833d182d705c9e8c3108e7cfe132d61a06e9133ddR28 to point to your updated branch.

[kylegoetz]

Something I need to check. Under the hood, the Haskell implementations of UDPSocket and ListenSocket wrap Network.Socket. I think that the TCP socket built-ins operate directly on Network.Socket (rather than a type like TCPSocket wrapping Network.Socket), which means that one could:

udpSocket = clientSocket host port -- UDPSocket
underlyingSocket = socket udpSocket -- Socket
base.IO.net.receive underlyingSocket -- not sure what happens here, but it typechecks!

The fact that we can unwrap Socket from UDPSocket and use the existing port to extract the Port from a Socket is nice, but it's a problem because Unison Share explicitly defines Socket as a TCP socket, which means a user might think they're extracting a TCP socket out of the UDPSocket type.

As an aside, most of the JIT Racket code is written now. I need to test it, plus figure out what the code meant to extract Socket from UDPSocket should do given the above notes.