+клиент, +сервер - #23
Palezehvat wants to merge 7 commits into
Conversation
| { | ||
| while (!_cancellationToken.IsCancellationRequested) | ||
| { | ||
| var client = await _listener.AcceptTcpClientAsync(); |
There was a problem hiding this comment.
Здесь нужно и в AcceptTcpClientAsync передавать cancellationToken, иначе listener никогда непозволит серверу остановиться, если к серверу никто не подключиться
| private static TcpListener? _listener; | ||
| private static List<TcpClient>? _clients; | ||
| private static CancellationTokenSource? _cancellationToken; | ||
| private static List<Task>? _tasks; |
There was a problem hiding this comment.
Зачем подчеркивания? :)
В Питоне, например, модификаторов видимости нет и это обосновано.
Здесь с маленькой буквы -- поле, с большой - свойство
|
|
||
| if (!_cancellationToken.IsCancellationRequested) | ||
| { | ||
| var sizeCommand = await stream.ReadAsync(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
Лучше сразу читать по строке, чтобы следовать протоколу (команды разделяются символом перевода строки) и не убирать потом \n
| var command = new StringBuilder(); | ||
| command.Append(Encoding.UTF8.GetString(buffer, 0, sizeCommand)); | ||
|
|
||
| var stringCommand = command.ToString(); |
There was a problem hiding this comment.
Зачем здесь StringBuilder? Можно все то же самое без него сделать включая получение первого символа строки
| foreach (var client in _clients) | ||
| { | ||
| client.Dispose(); | ||
| client.Close(); |
There was a problem hiding this comment.
Соединения лучше сразу закрывать после того, как сервер ответил. Иначе остаются бесполезные открытые соединения, в которые никто не пишет и из которых никто не читает
| return; | ||
| } | ||
|
|
||
| var serverThread = new Thread(async () => await server.Start()); |
There was a problem hiding this comment.
Не, это низкоуровневая многопоточность. В обычной жизни (когда не надо писать тредпул) лучше использовать более высокоуровневые Task-и.
Здесь, например, Task.Run.
И надо останавливать сервер в конце теста, если он такой метод (stop) предоставляет
| throw new ArgumentNullException(); | ||
| } | ||
|
|
||
| var client = new TcpClient(_hostname, _port); |
There was a problem hiding this comment.
Не, это инициирует синхронное подключение. Тут надо использовать конструктор без параметров и далее ConnectToAsync
|
|
||
| var stream = client.GetStream(); | ||
|
|
||
| await stream.WriteAsync(Encoding.UTF8.GetBytes("2 " + filePath + "\n")); |
There was a problem hiding this comment.
| await stream.WriteAsync(Encoding.UTF8.GetBytes("2 " + filePath + "\n")); | |
| await stream.WriteAsync(Encoding.UTF8.GetBytes($"2 {filePath}\n")); |
| } | ||
|
|
||
| private static async Task<string> GetResultFromStream(NetworkStream stream, string method) | ||
| { |
There was a problem hiding this comment.
Зачем передавать method, если он никак не используется? :)
И здесь некоторое неследование протоколу: считывать ответ надо до символа конца строки в случае list-запроса; считывать сначала длину ответа, а потом ответ, используя эту длину, для get-запроса.
| /// <param name="filePath">The relative path of the file from the specified path on the server</param> | ||
| public async Task<string> Get(string filePath) | ||
| { | ||
| if (_hostname == null) |
There was a problem hiding this comment.
Кстати, в случае Get запроса лучше передавать от вызывающего поток и туда считывать содержимое файла, полученное от сервера. Либо с помощью CopyToAsync, либо с помощью небольшого промежуточного буффера.
Чтобы файл сразу в память не читать, так как он большой может быть. Пусть вызывающий решает, что с ним делать
| Task.WaitAll(tasks.ToArray()); | ||
| foreach (var client in clients) | ||
| { | ||
| client.Close(); |
There was a problem hiding this comment.
"Соединения лучше сразу закрывать после того, как сервер ответил" -- не исправлено
| var textBytes = File.ReadAllBytes(path); | ||
| string text = Encoding.UTF8.GetString(textBytes); | ||
| string newText = $"{textBytes.Length} {text}"; | ||
| await stream.WriteAsync(Encoding.UTF8.GetBytes(newText)); |
There was a problem hiding this comment.
Не, стало только хуже :)
Чтобы не совершать пребразование байты -> строка -> байты, можно сначала отправить длину сообщения (с помощью отдельного WriteAsync), потом текст (с помощью отдельного WriteAsync), а потом сделать Flush
| class Program | ||
| { | ||
| static async Task Main() |
| { | ||
| var buffer = new byte[4096]; | ||
|
|
||
| var sizeResult = await stream.ReadAsync(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
Уже лучше, но здесь всё-таки надо сначала считывать длину ответа (до пробела), а потом ответ, используя эту длину
| /// </summary> | ||
| /// <param name="filePath">The relative path of the file from the specified path on the server</param> | ||
| public async Task<string> Get(string filePath) | ||
| { |
There was a problem hiding this comment.
Замечание про запись в поток актуально :)
| [TearDown] | ||
| public void Teardown() | ||
| { | ||
| server.Stop(); |
There was a problem hiding this comment.
Тесты падают. Похоже потому что server.Stop() единожды запрашивает остановку, и больше в силу реализации этот сервер уже запуститься не может
Одно из решений: запускать для каждого теста свой сервер на своем порту
3 participants
No description provided.