diff --git a/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs b/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs index a7e5396eb..5777a0af7 100644 --- a/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs +++ b/MediaBrowser.Server.Implementations/EntryPoints/ExternalPortForwarding.cs @@ -3,6 +3,7 @@ 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; @@ -10,9 +11,6 @@ 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 { @@ -22,8 +20,9 @@ namespace MediaBrowser.Server.Implementations.EntryPoints private readonly ILogger _logger; private readonly IServerConfigurationManager _config; private readonly ISsdpHandler _ssdp; - private CancellationTokenSource _currentCancellationTokenSource; - private TimeSpan _interval = TimeSpan.FromHours(1); + + private PeriodicTimer _timer; + private bool _isStarted; public ExternalPortForwarding(ILogManager logmanager, IServerApplicationHost appHost, IServerConfigurationManager config, ISsdpHandler ssdp) { @@ -31,78 +30,157 @@ namespace MediaBrowser.Server.Implementations.EntryPoints _appHost = appHost; _config = config; _ssdp = ssdp; - - _config.ConfigurationUpdated += _config_ConfigurationUpdated; } - private void _config_ConfigurationUpdated(object sender, EventArgs e) + 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) + { + if (!string.Equals(_lastConfigIdentifier, GetConfigIdentifier(), StringComparison.OrdinalIgnoreCase)) + { + if (_isStarted) + { + DisposeNat(); + } + + Run(); + } } public void Run() { - Discover(); - } - - private async void Discover() - { - if (!_config.Configuration.EnableUPnP) - { - return; - } - - var discoverer = new NatDiscoverer(); - - var cancellationTokenSource = new CancellationTokenSource(10000); - _currentCancellationTokenSource = cancellationTokenSource; - - try - { - var device = await discoverer.DiscoverDeviceAsync(PortMapper.Upnp, cancellationTokenSource).ConfigureAwait(false); - - await CreateRules(device).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - - } - catch (Exception ex) - { - _logger.ErrorException("Error discovering NAT devices", ex); - } - finally - { - _currentCancellationTokenSource = null; - } + //NatUtility.Logger = new LogWriter(_logger); if (_config.Configuration.EnableUPnP) { - await Task.Delay(_interval).ConfigureAwait(false); - Discover(); + 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); } } - private async Task CreateRules(NatDevice device) + void NatUtility_UnhandledException(object sender, UnhandledExceptionEventArgs e) + { + var ex = e.ExceptionObject as Exception; + + 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); + } + } + + 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); + } + } + + private List _createdRules = new List(); + 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 - await CreatePortMap(device, _appHost.HttpPort, _config.Configuration.PublicPort).ConfigureAwait(false); - await CreatePortMap(device, _appHost.HttpsPort, _config.Configuration.PublicHttpsPort).ConfigureAwait(false); + 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); + } } - private async Task CreatePortMap(NatDevice device, int privatePort, int publicPort) + 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) + { + Description = _appHost.Name + }); + } - try - { - await device.CreatePortMapAsync(new Mapping(Protocol.Tcp, privatePort, publicPort, _appHost.Name)).ConfigureAwait(false); - } - catch (Exception ex) - { - _logger.ErrorException("Error creating port map", ex); - } + // 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()); } public void Dispose() @@ -112,17 +190,39 @@ namespace MediaBrowser.Server.Implementations.EntryPoints private void DisposeNat() { - if (_currentCancellationTokenSource != null) + _logger.Debug("Stopping NAT discovery"); + + if (_timer != null) { - try - { - _currentCancellationTokenSource.Cancel(); - } - catch (Exception ex) - { - _logger.ErrorException("Error calling _currentCancellationTokenSource.Cancel", ex); - } + _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; } } } -} +} \ No newline at end of file diff --git a/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj b/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj index ae39d3eb9..60d8f737f 100644 --- a/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj +++ b/MediaBrowser.Server.Implementations/MediaBrowser.Server.Implementations.csproj @@ -62,10 +62,6 @@ ..\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 814a67643..66aede029 100644 --- a/MediaBrowser.Server.Implementations/packages.config +++ b/MediaBrowser.Server.Implementations/packages.config @@ -7,7 +7,6 @@ - \ No newline at end of file