Cleanup port forwarding service

- Use a concurrent collection instead of manually locking
- Do not forward HTTPS port when it is not enabled
- Created multiple rules (HTTP/HTTPS) in parallel instead of in sync

1 files changed, 33 insertions, 46 deletions

diff --git a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs b/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs
index e290c62e1..5525401c5 100644
--- a/Emby.Server.Implementations/EntryPoints/ExternalPortForwarding.cs
+++ b/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,8 +27,8 @@ 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;
 
@@ -98,7 +99,7 @@ namespace Emby.Server.Implementations.EntryPoints
             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;
 
@@ -117,26 +118,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 +135,7 @@ namespace Emby.Server.Implementations.EntryPoints
             }
         }
 
-        private async void CreateRules(INatDevice device)
+        private Task CreateRules(INatDevice device)
         {
             if (_disposed)
             {
@@ -153,50 +144,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 />