switch from Mono.Nat to Open.Nat

MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs

@@ -3,7 +3,6 @@ using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Controller.Dlna;
 using MediaBrowser.Controller.Plugins;
 using MediaBrowser.Model.Logging;
-using Mono.Nat;
 using System;
 using System.Collections.Generic;
 using System.Globalization;
@@ -11,6 +10,9 @@ using System.IO;
 using System.Net;
 using System.Text;
 using MediaBrowser.Common.Threading;
+using Open.Nat;
+using System.Threading;
+using System.Threading.Tasks;
 
 namespace MediaBrowser.Server.Implementations.EntryPoints
 {
@@ -20,9 +22,8 @@ namespace MediaBrowser.Server.Implementations.EntryPoints
         private readonly ILogger _logger;
         private readonly IServerConfigurationManager _config;
         private readonly ISsdpHandler _ssdp;
-
-        private PeriodicTimer _timer;
-        private bool _isStarted;
+        private CancellationTokenSource _currentCancellationTokenSource;
+        private TimeSpan _interval = TimeSpan.FromHours(1);
 
         public ExternalPortForwarding(ILogManager logmanager, IServerApplicationHost appHost, IServerConfigurationManager config, ISsdpHandler ssdp)
         {
@@ -32,223 +33,89 @@ namespace MediaBrowser.Server.Implementations.EntryPoints
             _ssdp = ssdp;
         }
 
-        private string _lastConfigIdentifier;
-        private string GetConfigIdentifier()
-        {
-            var values = new List<string>();
-            var config = _config.Configuration;
-
-            values.Add(config.EnableUPnP.ToString());
-            values.Add(config.PublicPort.ToString(CultureInfo.InvariantCulture));
-            values.Add(_appHost.HttpPort.ToString(CultureInfo.InvariantCulture));
-            values.Add(_appHost.HttpsPort.ToString(CultureInfo.InvariantCulture));
-            values.Add(config.EnableHttps.ToString());
-            values.Add(_appHost.EnableHttps.ToString());
-
-            return string.Join("|", values.ToArray());
-        }
-
-        void _config_ConfigurationUpdated(object sender, EventArgs e)
-        {
-            _config.ConfigurationUpdated -= _config_ConfigurationUpdated;
-
-            if (!string.Equals(_lastConfigIdentifier, GetConfigIdentifier(), StringComparison.OrdinalIgnoreCase))
-            {
-                if (_isStarted)
-                {
-                    DisposeNat();
-                }
-
-                Run();
-            }
-        }
-
         public void Run()
         {
             //NatUtility.Logger = new LogWriter(_logger);
 
             if (_config.Configuration.EnableUPnP)
             {
-                Start();
+                Discover();
             }
-
-            _config.ConfigurationUpdated -= _config_ConfigurationUpdated;
-            _config.ConfigurationUpdated += _config_ConfigurationUpdated;
         }
 
-        private void Start()
+        private async void Discover()
         {
-            _logger.Debug("Starting NAT discovery");
-            NatUtility.EnabledProtocols = new List<NatProtocol>
-            {
-                NatProtocol.Pmp
-            };
-            NatUtility.DeviceFound += NatUtility_DeviceFound;
-
-            // Mono.Nat does never rise this event. The event is there however it is useless. 
-            // You could remove it with no risk. 
-            NatUtility.DeviceLost += NatUtility_DeviceLost;
-
-
-            // it is hard to say what one should do when an unhandled exception is raised
-            // because there isn't anything one can do about it. Probably save a log or ignored it.
-            NatUtility.UnhandledException += NatUtility_UnhandledException;
-            NatUtility.StartDiscovery();
-
-            _timer = new PeriodicTimer(s => _createdRules = new List<string>(), null, TimeSpan.FromMinutes(5), TimeSpan.FromMinutes(5));
-
-            _ssdp.MessageReceived += _ssdp_MessageReceived;
+            var discoverer = new NatDiscoverer();
 
-            _lastConfigIdentifier = GetConfigIdentifier();
+            var cancellationTokenSource = new CancellationTokenSource(10000);
+            _currentCancellationTokenSource = cancellationTokenSource;
 
-            _isStarted = true;
-        }
-
-        void _ssdp_MessageReceived(object sender, SsdpMessageEventArgs e)
-        {
-            var endpoint = e.EndPoint as IPEndPoint;
-
-            if (endpoint != null && e.LocalEndPoint != null)
+            try
             {
-                NatUtility.Handle(e.LocalEndPoint.Address, e.Message, endpoint, NatProtocol.Upnp);
-            }
-        }
-
-        void NatUtility_UnhandledException(object sender, UnhandledExceptionEventArgs e)
-        {
-            var ex = e.ExceptionObject as Exception;
+                var device = await discoverer.DiscoverDeviceAsync(PortMapper.Upnp, cancellationTokenSource).ConfigureAwait(false);
 
-            if (ex == null)
-            {
-                //_logger.Error("Unidentified error reported by Mono.Nat");
+                await CreateRules(device).ConfigureAwait(false);
             }
-            else
+            catch (OperationCanceledException)
             {
-                // Seeing some blank exceptions coming through here
-                //_logger.ErrorException("Error reported by Mono.Nat: ", ex);
-            }
-        }
-
-        void NatUtility_DeviceFound(object sender, DeviceEventArgs e)
-        {
-            try
-            {
-                var device = e.Device;
-                _logger.Debug("NAT device found: {0}", device.LocalAddress.ToString());
 
-                CreateRules(device);
             }
             catch (Exception ex)
             {
-                // I think it could be a good idea to log the exception because 
-                //   you are using permanent portmapping here (never expire) and that means that next time
-                //   CreatePortMap is invoked it can fails with a 718-ConflictInMappingEntry or not. That depends
-                //   on the router's upnp implementation (specs says it should fail however some routers don't do it)
-                //   It also can fail with others like 727-ExternalPortOnlySupportsWildcard, 728-NoPortMapsAvailable
-                // and those errors (upnp errors) could be useful for diagnosting.  
-
-                // Commenting out because users are reporting problems out of our control
-                //_logger.ErrorException("Error creating port forwarding rules", ex);
+                _logger.ErrorException("Error discovering NAT devices", ex);
             }
-        }
-
-        private List<string> _createdRules = new List<string>();
-        private void CreateRules(INatDevice device)
-        {
-            // On some systems the device discovered event seems to fire repeatedly
-            // This check will help ensure we're not trying to port map the same device over and over
-
-            var address = device.LocalAddress.ToString();
-
-            if (!_createdRules.Contains(address))
+            finally
             {
-                _createdRules.Add(address);
-
-                CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort);
-                CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort);
+                _currentCancellationTokenSource = null;
             }
-        }
 
-        private void CreatePortMap(INatDevice device, int privatePort, int publicPort)
-        {
-            _logger.Debug("Creating port map on port {0}", privatePort);
-            device.CreatePortMap(new Mapping(Protocol.Tcp, privatePort, publicPort)
+            if (_config.Configuration.EnableUPnP)
             {
-                Description = _appHost.Name
-            });
+                await Task.Delay(_interval).ConfigureAwait(false);
+                Discover();
+            }
         }
 
-        // As I said before, this method will be never invoked. You can remove it.
-        void NatUtility_DeviceLost(object sender, DeviceEventArgs e)
+        private async Task CreateRules(NatDevice device)
         {
-            var device = e.Device;
-            _logger.Debug("NAT device lost: {0}", device.LocalAddress.ToString());
-        }
+            // On some systems the device discovered event seems to fire repeatedly
+            // This check will help ensure we're not trying to port map the same device over and over
 
-        public void Dispose()
-        {
-            DisposeNat();
+            await CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort).ConfigureAwait(false);
+            await CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort).ConfigureAwait(false);
         }
 
-        private void DisposeNat()
+        private async Task CreatePortMap(NatDevice device, int privatePort, int publicPort)
         {
-            _logger.Debug("Stopping NAT discovery");
-
-            if (_timer != null)
-            {
-                _timer.Dispose();
-                _timer = null;
-            }
-
-            _ssdp.MessageReceived -= _ssdp_MessageReceived;
+            _logger.Debug("Creating port map on port {0}", privatePort);
 
             try
             {
-                // This is not a significant improvement
-                NatUtility.StopDiscovery();
-                NatUtility.DeviceFound -= NatUtility_DeviceFound;
-                NatUtility.DeviceLost -= NatUtility_DeviceLost;
-                NatUtility.UnhandledException -= NatUtility_UnhandledException;
+                await device.CreatePortMapAsync(new Mapping(Protocol.Tcp, privatePort, publicPort, _appHost.Name)).ConfigureAwait(false);
             }
-            // Statements in try-block will no fail because StopDiscovery is a one-line 
-            // method that was no chances to fail.
-            //		public static void StopDiscovery ()
-            //      {
-            //          searching.Reset();
-            //      }
-            // IMO you could remove the catch-block
             catch (Exception ex)
             {
-                _logger.ErrorException("Error stopping NAT Discovery", ex);
-            }
-            finally
-            {
-                _isStarted = false;
+                _logger.ErrorException("Error creating port map", ex);
             }
         }
 
-        private class LogWriter : TextWriter
+        public void Dispose()
         {
-            private readonly ILogger _logger;
-
-            public LogWriter(ILogger logger)
-            {
-                _logger = logger;
-            }
-
-            public override Encoding Encoding
-            {
-                get { return Encoding.UTF8; }
-            }
-
-            public override void WriteLine(string format, params object[] arg)
-            {
-                _logger.Debug(format, arg);
-            }
+            DisposeNat();
+        }
 
-            public override void WriteLine(string value)
+        private void DisposeNat()
+        {
+            if (_currentCancellationTokenSource != null)
             {
-                _logger.Debug(value);
+                try
+                {
+                    _currentCancellationTokenSource.Cancel();
+                }
+                catch (Exception ex)
+                {
+                    _logger.ErrorException("Error calling _currentCancellationTokenSource.Cancel", ex);
+                }
             }
         }
     }

MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj

@@ -62,6 +62,10 @@
     <Reference Include="MoreLinq">
       <HintPath>..\packages\morelinq.1.4.0\lib\net35\MoreLinq.dll</HintPath>
     </Reference>
+    <Reference Include="Open.Nat, Version=2.0.15.0, Culture=neutral, PublicKeyToken=f22a6a4582336c76, processorArchitecture=MSIL">
+      <HintPath>..\packages\Open.NAT.2.0.15.0\lib\net45\Open.Nat.dll</HintPath>
+      <Private>True</Private>
+    </Reference>
     <Reference Include="Patterns.Logging">
       <HintPath>..\packages\Patterns.Logging.1.0.0.2\lib\portable-net45+sl4+wp71+win8+wpa81\Patterns.Logging.dll</HintPath>
     </Reference>

MediaBrowser.Server.Implementations/packages.config

@@ -7,6 +7,7 @@
   <package id="MediaBrowser.Naming" version="1.0.0.49" targetFramework="net45" />
   <package id="Mono.Nat" version="1.2.24.0" targetFramework="net45" />
   <package id="morelinq" version="1.4.0" targetFramework="net45" />
+  <package id="Open.NAT" version="2.0.15.0" targetFramework="net45" />
   <package id="Patterns.Logging" version="1.0.0.2" targetFramework="net45" />
   <package id="SocketHttpListener" version="1.0.0.29" targetFramework="net45" />
 </packages>