Good usage of MailboxProcessor/Agent in class rather than mutable field?

Question

I've written a simple Simple Service Discovery Protocol (SSDP) module. This module contains a class, Session that you create and then use to issue commands and subscribe to interesting events. I include the relevant code snippets below, but you can also view the entire SSDP module gist.

The usage looks like this:

//Fire up a session.
let ssdpSession = Ssdp.Session.Start(1985)
//Startlistening 
ssdpSession.MessageReceived.Add(fun msg -> printfn "%s" msg)
//Do an MSearch.
let search = { MSearch.SearchTarget = All; DeviceResponseDelay = OneSecond; }
ssdpSession.BroadcastMSearch(search)

It is intended that you hang on to the Session instance for as long as you need it. However, the Session is listening on one or more IP addresses for the lifetime of your usage, and these IP addresses may change (for example, you renew an IP DHCP lease). The Session class uses System.Net.NetworkInformation.NetworkChange.NetworkAddressChanged to detect this.

The question: What would be the elegant/idiomatic options to handle the changing of the IP and hence UdpClient listener set? I am trying to avoid mutable fields, but I wonder if I'm going too far with my approach below (just tell me, am I an astronaut!?). Are there elegant/idiomatic approaches for this?

What I have done so far, is run an MailboxProcessor (Agent) that fires up the UdpClient listeners and then waits for a message to fire up a new set (or tear them all down for good). The current set is passed to the recursive function running in the agent, so we always have it to hand when a change is requested or it is time to kill things off.

It looks like this:

let udpClientListeningController =
        Agent.Start(fun inbox -> 

            let rec loop (runningClients:UdpClient seq) = async {
                let! msg = inbox.Receive()

                let closeRunningClients () = 
                    runningClients |> Seq.iter (fun client -> client.ProtectedClose())

                match msg with

                | ListenForSearchResponsesAndNotifyAdverts (port, ipAddresses) ->

                    closeRunningClients ()

                    let uniAndMulticastUdpClients = seq {
                        let portIPPairs = ipAddresses |> Seq.zip (seq { yield port })
                        yield! portIPPairs |> Seq.map createUnicastClient
                        yield! portIPPairs |> Seq.map (snd >> createMulticastClient)
                    }

                    //Start em up.
                    uniAndMulticastUdpClients
                    |> Seq.iter (udpClientReceiveLoopAsync >> Async.Start)

                    return! loop uniAndMulticastUdpClients

                | StopListening ->
                    closeRunningClients ()
                    return! loop Seq.empty
            }
            loop Seq.empty 
        )

Answer 1

Your code looks pretty good to me - I would only change some minor details.

• I'd replace UdpClient seq with UdpClient list, because lazy sequences can be subtle. In your code, this doesn't seem to be a problem, but they can cause unexpected GC issues (by capturing a reference) and tricky performance behavior (by re-evaluating things).

• I'd probably write the creation of clients differently - I think the following is easier to understand:

  let uniAndMulticastUdpClients =
    [ for ip in ipAddresses do
        yield createUnicastClient(ip, port)
        yield createMulticastClient(ip) ]
  

• You're calling closeRunningClients in both branches, so you can extract it and move it before the match construct (and, in fact, you can just use the Seq.iter call without defining a function for it).