From d5b7ed325e5930b30fd8ecf54088d1bccdd2e8ba Mon Sep 17 00:00:00 2001 From: Luke Pulverenti Date: Thu, 31 Mar 2016 14:46:03 -0400 Subject: [PATCH] switch from Mono.Nat to Open.Nat --- .../EntryPoints/ExternalPortForwarding.cs | 227 ++++-------------- ...MediaBrowser.Server.Implementations.csproj | 4 + .../packages.config | 1 + 3 files changed, 52 insertions(+), 180 deletions(-) diff --git a/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs b/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs index 95763c43fc..c0c6264387 100644 --- a/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs +++ b/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,157 +33,70 @@ namespace MediaBrowser.Server.Implementations.EntryPoints _ssdp = ssdp; } - private string _lastConfigIdentifier; - private string GetConfigIdentifier() - { - var values = new List(); - 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(); - } - - _config.ConfigurationUpdated -= _config_ConfigurationUpdated; - _config.ConfigurationUpdated += _config_ConfigurationUpdated; - } - - private void Start() - { - _logger.Debug("Starting NAT discovery"); - NatUtility.EnabledProtocols = new List - { - 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(), null, TimeSpan.FromMinutes(5), TimeSpan.FromMinutes(5)); - - _ssdp.MessageReceived += _ssdp_MessageReceived; - - _lastConfigIdentifier = GetConfigIdentifier(); - - _isStarted = true; - } - - void _ssdp_MessageReceived(object sender, SsdpMessageEventArgs e) - { - var endpoint = e.EndPoint as IPEndPoint; - - if (endpoint != null && e.LocalEndPoint != null) - { - NatUtility.Handle(e.LocalEndPoint.Address, e.Message, endpoint, NatProtocol.Upnp); + Discover(); } } - void NatUtility_UnhandledException(object sender, UnhandledExceptionEventArgs e) + private async void Discover() { - var ex = e.ExceptionObject as Exception; + var discoverer = new NatDiscoverer(); - if (ex == null) - { - //_logger.Error("Unidentified error reported by Mono.Nat"); - } - else - { - // Seeing some blank exceptions coming through here - //_logger.ErrorException("Error reported by Mono.Nat: ", ex); - } - } + var cancellationTokenSource = new CancellationTokenSource(10000); + _currentCancellationTokenSource = cancellationTokenSource; - void NatUtility_DeviceFound(object sender, DeviceEventArgs e) - { try { - var device = e.Device; - _logger.Debug("NAT device found: {0}", device.LocalAddress.ToString()); + var device = await discoverer.DiscoverDeviceAsync(PortMapper.Upnp, cancellationTokenSource).ConfigureAwait(false); + + await CreateRules(device).ConfigureAwait(false); + } + catch (OperationCanceledException) + { - 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. + _logger.ErrorException("Error discovering NAT devices", ex); + } + finally + { + _currentCancellationTokenSource = null; + } - // Commenting out because users are reporting problems out of our control - //_logger.ErrorException("Error creating port forwarding rules", ex); + if (_config.Configuration.EnableUPnP) + { + await Task.Delay(_interval).ConfigureAwait(false); + Discover(); } } - private List _createdRules = new List(); - private void CreateRules(INatDevice device) + private async Task CreateRules(NatDevice 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)) - { - _createdRules.Add(address); - - CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort); - CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort); - } + await CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort).ConfigureAwait(false); + await CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort).ConfigureAwait(false); } - private void CreatePortMap(INatDevice device, int privatePort, int publicPort) + private async Task CreatePortMap(NatDevice device, int privatePort, int publicPort) { _logger.Debug("Creating port map on port {0}", privatePort); - device.CreatePortMap(new Mapping(Protocol.Tcp, privatePort, publicPort) - { - Description = _appHost.Name - }); - } - // As I said before, this method will be never invoked. You can remove it. - void NatUtility_DeviceLost(object sender, DeviceEventArgs e) - { - var device = e.Device; - _logger.Debug("NAT device lost: {0}", device.LocalAddress.ToString()); + try + { + await device.CreatePortMapAsync(new Mapping(Protocol.Tcp, privatePort, publicPort, _appHost.Name)).ConfigureAwait(false); + } + catch (Exception ex) + { + _logger.ErrorException("Error creating port map", ex); + } } public void Dispose() @@ -192,63 +106,16 @@ namespace MediaBrowser.Server.Implementations.EntryPoints private void DisposeNat() { - _logger.Debug("Stopping NAT discovery"); - - if (_timer != null) + if (_currentCancellationTokenSource != null) { - _timer.Dispose(); - _timer = null; - } - - _ssdp.MessageReceived -= _ssdp_MessageReceived; - - try - { - // This is not a significant improvement - NatUtility.StopDiscovery(); - NatUtility.DeviceFound -= NatUtility_DeviceFound; - NatUtility.DeviceLost -= NatUtility_DeviceLost; - NatUtility.UnhandledException -= NatUtility_UnhandledException; - } - // 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; - } - } - - private class LogWriter : TextWriter - { - 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); - } - - public override void WriteLine(string value) - { - _logger.Debug(value); + try + { + _currentCancellationTokenSource.Cancel(); + } + catch (Exception ex) + { + _logger.ErrorException("Error calling _currentCancellationTokenSource.Cancel", ex); + } } } } diff --git a/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj b/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj index 97f090ab2a..84c1cae544 100644 --- a/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj +++ b/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj @@ -62,6 +62,10 @@ ..\packages\morelinq.1.4.0\lib\net35\MoreLinq.dll + + ..\packages\Open.NAT.2.0.15.0\lib\net45\Open.Nat.dll + True + ..\packages\Patterns.Logging.1.0.0.2\lib\portable-net45+sl4+wp71+win8+wpa81\Patterns.Logging.dll diff --git a/MediaBrowser.Server.Implementations/packages.config b/MediaBrowser.Server.Implementations/packages.config index 66aede029f..814a676437 100644 --- a/MediaBrowser.Server.Implementations/packages.config +++ b/MediaBrowser.Server.Implementations/packages.config @@ -7,6 +7,7 @@ + \ No newline at end of file