-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make FtpServer disposable #10
Comments
The resources (TcpListener) held by FtpServer should be released when the CancellationToken in FtpServer.RunAsync() has a cancel request. There will be nothing to dispose. |
@taoyouh how do you think, which code is more clean and obvious? [Fact]
public async Task Test()
{
// arrange
var endPoint = new IPEndPoint(IPAddress.Loopback, 21);
var ftpServer = new FtpServer(endPoint, Directory.GetCurrentDirectory());
var cancellationTokenSource = new CancellationTokenSource();
await ftpServer.RunAsync(cancellationTokenSource.Token);
var ftpServerCaller = new FtpServerCaller();
// act
try
{
await ftpServerCaller.CallFtpServer();
}
finally
{
cancellationTokenSource.Cancel();
}
// assert
ftpServerCaller.CheckResult();
} or [Fact]
public async Task Test()
{
// arrange
var endPoint = new IPEndPoint(IPAddress.Loopback, 21);
using var ftpServer = new FtpServer(endPoint, Directory.GetCurrentDirectory());
await ftpServer.RunAsync();
var ftpServerCaller = new FtpServerCaller();
// act
await ftpServerCaller.CallFtpServer();
// assert
ftpServerCaller.CheckResult();
} |
Both version of your code will not work. You have to be running RunAsync to accept a connection. You cannot just await it (it will not complete unless you cancel it). Maybe you prefer a .Start() and .StopAsync() pattern rather than the current Run() pattern, but that will need a new thread be created to run the main server loop. A simple wrapper could be written on the client side to implement this. If we must use IDisposable pattern, we need to find a way to stop all existing connections synchronously (because Dispose is not async). |
No description provided.
The text was updated successfully, but these errors were encountered: