1
\$\begingroup\$

I have a set of components that each need to both publish and receive messages using a multicast channel. In production, any one machine could host two or more of these components with 12 or more machines communicating with each other.

My question is... given that components on the same machine and components on separate machines are both sending and received. Have I set this up in the most efficient manner? (Note: messages can periodically be lost with no ill effect)

private readonly static Socket _broadcastSocket;

private readonly static Socket _broadcastListeningSocket;

_broadcastSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);

_broadcastListeningSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);

IPAddress ip = IPAddress.Parse(_configuration.Server.AddressV4);
var ipep = new IPEndPoint(ip, _configuration.Server.Port);

_broadcastSocket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, new MulticastOption(ip));
_broadcastSocket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.MulticastTimeToLive, _configuration.Server.TimeToLive);
_broadcastSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ExclusiveAddressUse, false);
_broadcastSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, 1);
_broadcastSocket.Connect(ipep);

var listenerIpep = new IPEndPoint(IPAddress.Any, _configuration.Server.Port);

_broadcastListeningSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ExclusiveAddressUse, false);
_broadcastListeningSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, 1);
_broadcastListeningSocket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, new MulticastOption(ip, IPAddress.Any));
_broadcastListeningSocket.Bind(listenerIpep);

Messages are received using this following:

_broadcastListeningSocket.Receive(payloadBuffer);

Messages are sent using the following:

if (_broadcastSocket.Connected)
{
    _broadcastSocket.Send(payload);
}

I'm a little new to this so I am assuming I am not setting all the options correctly, feel free to suggest setting of new options.

\$\endgroup\$
1
\$\begingroup\$

Unfortunately, I'm unable to comment about the actual options you are setting since

  • while I've used Sockets in Java, I haven't used them in C#
  • I'm finding it a bit hard to see what your code is doing because it isn't very expressive

So, while I can do nothing about the first point (someone with more experience will have to help you out), we can certainly remedy the second problem by refactoring your code into a more readable form with less duplication.

Let's start by declaring a new member variable to keep hold of the parsed IP address:

private static IPAddress _ipAddress = IPAddress.Parse(_configuration.Server.AddressV4);

We'll use this field in the methods that result from splitting up your monolithic function into several helpers, leaving us with the following:

_broadcastSocket = CreateBroadcastSocket();
_broadcastListeningSocket = CreateBroadcastListeningSocket(); 
_broadcastSocket.Connect(GetEndpointOf(_ipAddress));
_broadcastListeningSocket.Bind(GetEndpointOf(IPAddress.Any));

Those methods are defined as the following:

private static Socket CreateBroadcastSocket()
{
    Socket s = CreateSocket(new MulticastOption(_ipAddress));
    s.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.MulticastTimeToLive, _configuration.Server.TimeToLive);
    return s;
}

private static Socket CreateBroadcastListeningSocket()
{
    return CreateSocket(new MulticastOption(_ipAddress, IPAddress.Any));
}

private static Socket CreateSocket(MulticastOption multicastOption)
{
    Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
    s.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ExclusiveAddressUse, false);
    s.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, 1);
    s.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, multicastOption);
    return s;
}

Note that it would have been possible to set the instance variables (_broadcastListeningSocketand _broadcastSocket) directly from the helper methods, instead of returning a new Socket, however I find the above implementation more expressive because we seperate the concerns: The helper methods know about how to create a configured Socket, while the method that calls them knows about what they have to be assigned to.

What to do if you find out you need to set the options differently?

  • Create a new CreateXXXSocket() method
  • From that method, call CreateSocket() and
  • configure the result and pass it back to the caller
\$\endgroup\$
  • \$\begingroup\$ Thanks... I take your point. I'll certainly incorporate some of this. \$\endgroup\$ – JoeGeeky Dec 28 '11 at 11:49
  • \$\begingroup\$ I'm sorry I couldn't help with the subject itself... \$\endgroup\$ – Adam Dec 28 '11 at 11:55
  • \$\begingroup\$ Don't be sorry. This is a "Code Review" so comments such as yours are completely reasonable in my opinion (e.g. style, readability, clarity, etc...). \$\endgroup\$ – JoeGeeky Dec 28 '11 at 12:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.