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 
 )

• John Palmer: I would maybe cut down on the blank lines
• bentayloruk: Haha @JohnPalmer, I'm still trying to decide on my preferred "blank line" style. Particularly in a lambda and long match. Fancy forking the Gist and showing me your style?
• John Palmer: I think my style would be to have no blank lines like this:gist.github.com/jpalmer/d7149e84301b9e2586c6 but having a few more than I do is probably OK.
• bentayloruk: I think I'll adopt the no blank lines and see how I feel about it in a few months. Thanks for the Gist. I updated and took Tomas's suggestions here gist.github.com/bentayloruk/… (I think it looks a lot better).
• James Moore: The blank lines are good. I wouldn't take them out.

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).

• bentayloruk: Nice, thanks Tomas. Your code always look so good! I originally had the closeRunningClients code before the match, but then thought it might be less explicit and the introduction of an additional message in the match that didn't require a close would result in broken code. A touch defensive? :)
• bentayloruk: Oh, and Tomas, any specific comment on using a MailboxProcessor here rather than mutable or ref cell?
• Tomas Petricek: @bentayloruk Inside agent, you can safely use mutable data structures (as long as they don't leak out of the agent). In this case, it does not really matter. I'd use mutable if I want an efficient hashtable or something like that.
• bentayloruk: Yes, I understand that I can use them. My question was more about wether usage of the MailboxProcessor in the context of this class was elegant/idiomatic. You can see it in context here. @7sharp9 said he would typically use a ref cell for the list in this situation. What would you do?