Problem
I’m currently working on several projects for my company to help reduce the amount of calls we have to deal with so we can focus on higher priority task, such as server resource reduction, etc.
The first project on my list was to reduce the number of proxy issues a user has when migrating from an internal network to an external network such as home Internet.
I have come up with a Windows service idea where the service would be deployed from our servers and installed via a command. Once installed and started the service would monitor the connection states of all the network interface on the computer.
Upon a network interface change such as:
- Ethernet plugged out
- IP changes
- Interfaces enabled / disabled
- etc
the service would attempt to ping our DomainController
. If the ping is a success, we will then check to see if the proxy settings for the machine are set. If not, we will automatically set them and enable them. If the ping is unsuccessful, we will disable the proxy.
The project is not fully complete but there is still a nice bit of code there to review.
ProxyMonitor.cs
using System;
using System.Diagnostics;
using System.ServiceProcess;
using System.Configuration.Install;
using System.Reflection;
using System.Threading;
using System.Net.NetworkInformation;
namespace Serco.Services.ProxyMonitor
{
class ProxyMonitor : ServiceBase
{
static void Main(string[] args)
{
AppDomain.CurrentDomain.UnhandledException += CurrentDomainUnhandledException;
if (System.Environment.UserInteractive)
{
string parameter = string.Concat(args);
switch (parameter)
{
case "/install":
ManagedInstallerClass.InstallHelper(new string[] { Assembly.GetExecutingAssembly().Location });
break;
case "/uninstall":
ManagedInstallerClass.InstallHelper(new string[] { "/u", Assembly.GetExecutingAssembly().Location });
break;
}
}
else
{
ServiceBase.Run(new ProxyMonitor());
}
}
private static void CurrentDomainUnhandledException(object sender, UnhandledExceptionEventArgs e)
{
}
/*
* Start the main service application
*/
private ManualResetEvent MainShutdownEvent = new ManualResetEvent(false);
private Thread MainThread;
private static EventLog EventManager = new EventLog();
public static string ProxyIp;
public ProxyMonitor()
{
EventManager.Source = ServiceConfiguration.ServiceName;
}
protected override void OnStart(string[] args)
{
ProxyIp = string.Concat(args);
MainThread = new Thread(MainWorkerThread);
MainThread.Name = "MainWorkerThread";
MainThread.IsBackground = true;
MainThread.Start();
}
private void MainWorkerThread()
{
EventManager.WriteEntry("Watching for ip: " + ProxyIp);
NetworkChange.NetworkAddressChanged += new NetworkAddressChangedEventHandler(AddressChangedCallback);
}
public static void AddressChangedCallback(object Sender,EventArgs Args)
{
//try and ping the Proxy Server
Ping Ping = new Ping();
PingReply Reply = Ping.Send("secret.domain");
if (Reply.Status == IPStatus.Success)
{
/*
* Update Proxy Settings
*/
}
}
protected override void OnStop()
{
MainShutdownEvent.Set();
if(!MainThread.Join(3000))
{
// give the thread 3 seconds to stop
MainThread.Abort();
}
}
}
}
ManagedInstallation.cs
using System;
using System.ServiceProcess;
using System.Configuration;
using System.ComponentModel;
using System.Configuration.Install;
namespace Serco.Services.ProxyMonitor
{
[RunInstaller(true)]
public class ManagedInstallation : Installer
{
public ManagedInstallation()
{
var ProcessInstaller = new ServiceProcessInstaller();
var ServiceInstaller = new ServiceInstaller();
//set the information and privileges
ProcessInstaller.Account = ServiceConfiguration.AccountType;
ProcessInstaller.Username = null;
ProcessInstaller.Password = null;
ServiceInstaller.DisplayName = ServiceConfiguration.DisplayName;
ServiceInstaller.StartType = ServiceConfiguration.StartType;
ServiceInstaller.Description = ServiceConfiguration.Description;
ServiceInstaller.ServiceName = ServiceConfiguration.ServiceName;
Installers.Add(ProcessInstaller);
Installers.Add(ServiceInstaller);
}
private ServiceController ServiceController = new ServiceController(ServiceConfiguration.ServiceName);
protected override void OnAfterInstall(System.Collections.IDictionary savedState)
{
base.OnAfterInstall(savedState);
ServiceController.Start();
}
}
}
ServiceConfiguration.cs
using System;
using System.Collections.Generic;
using System.Text;
using System.ServiceProcess;
namespace Serco.Services.ProxyMonitor
{
class ServiceConfiguration
{
public static string DisplayName
{
get { return "Proxy Monitor"; }
}
public static string ServiceName
{
get { return "ProxyMonitor"; }
}
public static string Description
{
get
{
return "ProxyMonitor is a helper developed to manage the state of the proxy for the employess whilst of the internal network.";
}
}
public static ServiceStartMode StartType
{
get{return ServiceStartMode.Automatic;}
}
public static ServiceAccount AccountType
{
get{return ServiceAccount.LocalSystem;}
}
}
}
The ServiceConfiguration
class may be modified to populate from a local file depending on the deployment discussion I will have with my colleges.
Solution
Just a few points that came to mind as I was reading your code.
-
It appears to me that the main thread terminates once the
NetworkChange.NetworkAddressChanged
event is hooked, in which case the thread is redundant as the event will be raised by another thread anyway. -
MainShutdownEvent
is created and ‘set’ but I don’t see anything actually using it. -
NetworkChange.NetworkAddressChanged
is hooked but not unhooked, it won’t matter much when the appdomain is unloaded on termination but I would just feel better if it was part of the standard ‘shutdown’ process inOnStop()
-
I don’t like the empty
UnhandledException
event handler, if you are going to hook it I would suggest some form of logging. At a minimum you should use log such events to the event log via theEventLog
property onServiceBase
. -
I’m not sure which thread will be used to rais the
NetworkChange.NetworkAddressChanged
, but I wouldn’t rely on it being the same one asOnStop()
so you should probably look at some form of locking/signaling to handle any situations whereOnStop()
is called while the event handler is running. It might not cause any real issues but I personally would want to play it safe. It might be interesting to temporarily add a long sleep into the event handler just to see what happens if it’s busy when told to stop.
I would recommend to look at Exception Handling in your code – there is nothing except for empty CurrentDomainExceptionHandler. However it’s very crucial for such services to have a clear exception hierarchy and handling policy. There should be at least 2 types:
-
Non-recoverable errors – if service couldn’t start because of invalid configuration etc
-
Recoverable errors – if you couldn’t pong “secret.domain” because it’s unavailable then should you proceed? How to deal with timeout exceptions? Should there be a queue with unproceeded requests or should they immediately be discarded? Should the size of the queue be configurable if you need it? If you want your service to work in 24×7 mode then you must have answers on these and few other questions about how to deal with unexpected situations.
Another missing point is Configuration. Are you goint to rewrite your code and re-install service if “secret.domain” changes? Or if you need to increase timeout or even change display name for service? I would recommend to have a configuration file that can be re-read on every start. It would save you a lot time.
Consider using TopShelf to manage the whole install/uninstall/configure service task.
This has the wonderful feature of allowing you to debug your code from the command line instead of having to attach to a windows service.