Merge pull request #2775 from mark-monteiro/upnp-cleanup

Port Forwarding Improvements

Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs

@@ -1,6 +1,7 @@
 #pragma warning disable CS1591
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Net;
 using System.Text;
@@ -26,10 +27,10 @@ namespace Emby.Server.Implementations.EntryPoints
         private readonly IServerConfigurationManager _config;
         private readonly IDeviceDiscovery _deviceDiscovery;
 
-        private readonly object _createdRulesLock = new object();
-        private List<IPEndPoint> _createdRules = new List<IPEndPoint>();
+        private readonly ConcurrentDictionary<IPEndPoint, byte> _createdRules = new ConcurrentDictionary<IPEndPoint, byte>();
+
         private Timer _timer;
-        private string _lastConfigIdentifier;
+        private string _configIdentifier;
 
         private bool _disposed = false;
 
@@ -60,6 +61,7 @@ namespace Emby.Server.Implementations.EntryPoints
             return new StringBuilder(32)
                 .Append(config.EnableUPnP).Append(Separator)
                 .Append(config.PublicPort).Append(Separator)
+                .Append(config.PublicHttpsPort).Append(Separator)
                 .Append(_appHost.HttpPort).Append(Separator)
                 .Append(_appHost.HttpsPort).Append(Separator)
                 .Append(_appHost.EnableHttps).Append(Separator)
@@ -69,7 +71,10 @@ namespace Emby.Server.Implementations.EntryPoints
 
         private void OnConfigurationUpdated(object sender, EventArgs e)
         {
-            if (!string.Equals(_lastConfigIdentifier, GetConfigIdentifier(), StringComparison.OrdinalIgnoreCase))
+            var oldConfigIdentifier = _configIdentifier;
+            _configIdentifier = GetConfigIdentifier();
+
+            if (!string.Equals(_configIdentifier, oldConfigIdentifier, StringComparison.OrdinalIgnoreCase))
             {
                 Stop();
                 Start();
@@ -93,21 +98,19 @@ namespace Emby.Server.Implementations.EntryPoints
                 return;
             }
 
-            _logger.LogDebug("Starting NAT discovery");
+            _logger.LogInformation("Starting NAT discovery");
 
             NatUtility.DeviceFound += OnNatUtilityDeviceFound;
             NatUtility.StartDiscovery();
 
-            _timer = new Timer(ClearCreatedRules, null, TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10));
+            _timer = new Timer((_) => _createdRules.Clear(), null, TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10));
 
             _deviceDiscovery.DeviceDiscovered += OnDeviceDiscoveryDeviceDiscovered;
-
-            _lastConfigIdentifier = GetConfigIdentifier();
         }
 
         private void Stop()
         {
-            _logger.LogDebug("Stopping NAT discovery");
+            _logger.LogInformation("Stopping NAT discovery");
 
             NatUtility.StopDiscovery();
             NatUtility.DeviceFound -= OnNatUtilityDeviceFound;
@@ -117,26 +120,16 @@ namespace Emby.Server.Implementations.EntryPoints
             _deviceDiscovery.DeviceDiscovered -= OnDeviceDiscoveryDeviceDiscovered;
         }
 
-        private void ClearCreatedRules(object state)
-        {
-            lock (_createdRulesLock)
-            {
-                _createdRules.Clear();
-            }
-        }
-
         private void OnDeviceDiscoveryDeviceDiscovered(object sender, GenericEventArgs<UpnpDeviceInfo> e)
         {
             NatUtility.Search(e.Argument.LocalIpAddress, NatProtocol.Upnp);
         }
 
-        private void OnNatUtilityDeviceFound(object sender, DeviceEventArgs e)
+        private async void OnNatUtilityDeviceFound(object sender, DeviceEventArgs e)
         {
             try
             {
-                var device = e.Device;
-
-                CreateRules(device);
+                await CreateRules(e.Device).ConfigureAwait(false);
             }
             catch (Exception ex)
             {
@@ -144,7 +137,7 @@ namespace Emby.Server.Implementations.EntryPoints
             }
         }
 
-        private async void CreateRules(INatDevice device)
+        private Task CreateRules(INatDevice device)
         {
             if (_disposed)
             {
@@ -153,50 +146,46 @@ namespace Emby.Server.Implementations.EntryPoints
 
             // 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.DeviceEndpoint;
-
-            lock (_createdRulesLock)
+            if (!_createdRules.TryAdd(device.DeviceEndpoint, 0))
             {
-                if (!_createdRules.Contains(address))
-                {
-                    _createdRules.Add(address);
-                }
-                else
-                {
-                    return;
-                }
+                return Task.CompletedTask;
             }
 
-            try
-            {
-                await CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort).ConfigureAwait(false);
-            }
-            catch (Exception ex)
-            {
-                _logger.LogError(ex, "Error creating http port map");
-                return;
-            }
+            return Task.WhenAll(CreatePortMaps(device));
+        }
 
-            try
-            {
-                await CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort).ConfigureAwait(false);
-            }
-            catch (Exception ex)
+        private IEnumerable<Task> CreatePortMaps(INatDevice device)
+        {
+            yield return CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort);
+
+            if (_appHost.EnableHttps)
             {
-                _logger.LogError(ex, "Error creating https port map");
+                yield return CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort);
             }
         }
 
-        private Task<Mapping> CreatePortMap(INatDevice device, int privatePort, int publicPort)
+        private async Task CreatePortMap(INatDevice device, int privatePort, int publicPort)
         {
             _logger.LogDebug(
-                "Creating port map on local port {0} to public port {1} with device {2}",
+                "Creating port map on local port {LocalPort} to public port {PublicPort} with device {DeviceEndpoint}",
                 privatePort,
                 publicPort,
                 device.DeviceEndpoint);
 
-            return device.CreatePortMapAsync(
-                new Mapping(Protocol.Tcp, privatePort, publicPort, 0, _appHost.Name));
+            try
+            {
+                var mapping = new Mapping(Protocol.Tcp, privatePort, publicPort, 0, _appHost.Name);
+                await device.CreatePortMapAsync(mapping).ConfigureAwait(false);
+            }
+            catch (Exception ex)
+            {
+                _logger.LogError(
+                    ex,
+                    "Error creating port map on local port {LocalPort} to public port {PublicPort} with device {DeviceEndpoint}.",
+                    privatePort,
+                    publicPort,
+                    device.DeviceEndpoint);
+            }
         }
 
         /// <inheritdoc />

MediaBrowser.Model/Configuration/ServerConfiguration.cs

@@ -15,9 +15,8 @@ namespace MediaBrowser.Model.Configuration
         private string _baseUrl;
 
         /// <summary>
-        /// Gets or sets a value indicating whether [enable u pn p].
+        /// Gets or sets a value indicating whether to enable automatic port forwarding.
         /// </summary>
-        /// <value><c>true</c> if [enable u pn p]; otherwise, <c>false</c>.</value>
         public bool EnableUPnP { get; set; }
 
         /// <summary>
@@ -254,7 +253,7 @@ namespace MediaBrowser.Model.Configuration
             AutoRunWebApp = true;
             EnableRemoteAccess = true;
 
-            EnableUPnP = true;
+            EnableUPnP = false;
             MinResumePct = 5;
             MaxResumePct = 90;