Conversation
Refactor of http tracker test using a fake endpoint (SamuelFisher#6)
|
This is really cool. I hope this framework is not dead. CC @SamuelFisher. |
SamuelFisher
left a comment
SamuelFisher
left a comment
There was a problem hiding this comment.
Hi, apologies for the very long delay with looking at this.
This is a great addition to have UPnP support, however I understand if you're not interest in progressing this any further due to the delay - if so, please ignore my comments!
| <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'"> | ||
| <PlatformTarget>AnyCPU</PlatformTarget> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
This looks unnecessary, can you remove it?
TorrentCore/TorrentCore.csproj
Outdated
| <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.2.0" /> | ||
| <PackageReference Include="Microsoft.Extensions.Logging" Version="2.2.0" /> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.2.0" /> | ||
| <PackageReference Include="Open.NAT.Core" Version="2.1.0" /> |
There was a problem hiding this comment.
This library looks to be based on Mono.Nat which seems to have more usage. Would it be better to use the Mono library instead of this fork?
| var cts = new CancellationTokenSource(10000); | ||
|
|
||
| var device = await discoverer.DiscoverDeviceAsync(PortMapper.Upnp, cts); | ||
| await device.CreatePortMapAsync(new Mapping(Protocol.Tcp, port, port, "TorrentCore")); |
There was a problem hiding this comment.
This doesn't take into consideration which IP the application is listening on (TorrentClientSettings.AdapterAddress).
For context, AdapterAddress is used for situations such as forcing TorrentCore to run over a VPN connection. In this situation it could end up adding the mapping onto a network the user specifically doesn't want TorrentCore to be using.
| } | ||
| catch | ||
| { | ||
| // Do nothing |
There was a problem hiding this comment.
What type of exceptions does this throw? It could do with some logging to indicate something has gone wrong.
| _logger = logger; | ||
| _streams = new ConcurrentBag<TcpTransportStream>(); | ||
| Port = options.Value.Port; | ||
| _uPnPClient = new TcpTransportUPnPClient(options.Value.UseUPnP); |
There was a problem hiding this comment.
Could you register this in the DI container and inject it instead of creating it here? This will allow users more flexibility to change how UPnP is handled if required.
Thank you for your comments. I'll take an eye when I come from my holidays because I am in my parent's house wich have not an upnp capable router for testing. |
|
Hi @SamuelFisher, are you going to merge net6 version into master branch? I was thinking about to continue developing this feature and I don't know if would be better to abandon this Pr and start over net6 branch or wait until net6 branch is merged into master. |
4 participants
If feature is added in client configuration, automatically open port just when TCP transport start listening. It uses Open.Nat library for UPnP functionality.