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,223 +33,89 @@ 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(); + 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.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; + 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 _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 - - 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); + } } } } 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