Skip to content

Simple ftp#52

Open
IgnatSergeev wants to merge 15 commits intomainfrom
simple-ftp
Open

Simple ftp#52
IgnatSergeev wants to merge 15 commits intomainfrom
simple-ftp

Conversation

@IgnatSergeev
Copy link
Owner

No description provided.

using SimpleFtp.Protocol;

var server = new FtpServer();
await server.Listen();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь нужен механизм корректной остановки сервера. Например, по нажатию на какую-нибудь клавишу

Comment on lines 6 to 9
var response = client.SendRequest(request);
Console.Write(response.ToString());
response = client.SendRequest(request);
Console.Write(response.ToString());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

копипаст

namespace SimpleFtp;
public class FtpServer
{
private const int Port = 21;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь специально используется 21 порт, на котором обычный FTP работает? :)
Оригинально, но надо на другой заменить. Лучше на 1024+, потому что первые 1024 зарезервированы для часто используемых служб

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

И Port с маленькой, это же поле

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А это же const

public async Task Listen()
{
_listener.Start();
while (true)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Надо добавить возможность остановки сервера, использовать CancellationToken и передавать его в _listener.AcceptTcpClientAsync()

while (true)
{
using var client = await _listener.AcceptTcpClientAsync();
await HandleClient(client);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если await-ить работу с клиентами, то нарушается условие -- сервер не может параллельно несколько запросов обрабатывать. Ожидалась передача задачи работы с клиентами в отдельный поток, например, с помощью Task.Run

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Также надо будет не забывать эти задачи сохранять, чтобы завершать работу сервера только после закрытия всех соединений с клиентами


public abstract class Request
{
public abstract string ToString();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь аналогично предупреждения ниже

private StreamReader? _reader;
private StreamWriter? _writer;
public string Hostname { get; private set; }
public int Port { get; private set; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Свойства надо ниже конструктора перенести

}
}
}
} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тестов маловато

Console.Write($"Request: {data}");
var response = HandleRequest(RequestFactory.Create(data));
await writer.WriteAsync(response.ToString());
Console.Write($"Response: {response.ToString()}");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А это зачем?
Клиент же и так в своём program выводит ответ. Там же виден и запрос.
Надо стараться UI выносить вне реализации. Да и вряд ли пользователь захочет увидеть набор байтов какого-нибудь бинарника :)

public Response SendRequest(Request request)
{
_writer?.Write(request.ToString());
var data = _reader?.ReadLine() + "\n";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В случае get-запроса ответ некорректно считается: \n может раньше конца команды оказаться -- внутри содержимого файла.
Еще в случае get-запроса лучше передавать содержимое файла в поток, который предоставит пользователь. Чтобы файл напрямую в память не читать (так как он большой может быть), а предоставить право выбора вызывающему

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants