Comments 72
Ладно, если серьёзно, то никогда не используйте регулярки без таймаута. Об этом даже отдельное предупреждение в документации.
это одно из самых жестких что я видел на хабре ...
Нет, конечно можно было сделать проще:
new Task.Run(
()=> {
ClientThread(Listener.Accept());
}
);Но такой способ не эффективен, так как он будет просто создавать новые потоки для обработки входящего соединения, тем самым тормозя работу сервера, ведь как никак потоки у нашего процессора не безграничны.
Зачем new? Зачем фигурные скобки? Task.Run не создаёт поток на каждую задачу, а использует пул потоков, о чем сказано в MSDN:
Queues the specified work to run on the ThreadPool and returns a task or Task<TResult> handle for that work.
Теперь приступим к реализации многопоточности. Делать мы ее будем на основе такого класса как ThreadPool. Нет, конечно можно было сделать проще [...] Но такой способ не эффективен, так как он будет просто создавать новые потоки для обработки входящего соединения, тем самым тормозя работу сервера, ведь как никак потоки у нашего процессора не безграничны
Вообще-то, вариант через Task.Run тоже использует ThreadPool. Причина его неэффективноси — совсем в другом: вы передаёте блокирующую операцию (Accept) в другой поток, из-за чего теряете возможность узнать когда она закончится в потоке текущем (по умному это называется утерей обратного давления, backpressure).
Поэтому надо не играть с записыванием всего в одну строку, а отделить блокирующую операцию от создания задачи:
while (Active)
{
var client = Listener.Accept();
new Task.Run(()=> ClientThread(client));
}Теперь перейдем к нашей функции остановки сервера
А вот тут-то вы накосячили, и сильно: ваш метод остановки сервера никак не пытается ни дождаться окончания обработки всех входящих соединений, ни даже остановить их!
Для решения первой задачи вам надо запомнить все начатые задачи (вот почему Task использовать удобнее чем ThreadPool!), а для решения второй надо как-то передать сигнал об окончании обработки всем соединениям, и это удобно делать через CancellationToken;
В итоге надо снова менять процесс запуска сервера, и получается что-то вроде вот такого решения:
CancellationTokenSource ctsStop = new ();
List<Task> activeTasks = new ();
// также тут может быть HashSet, связный список или вовсе ConcurrentDictionary
// …
while (!ctsStop.IsCancellationRequested)
{
var client = Listener.Accept();
var task = new Task.Run(()=> ClientThread(client, ctsStop.Token));
lock(activeTasks) activeTasks.Add(task);
task.ContinueWith(t => {
lock(activeTasks) activeTasks.Remove(t);
}, TaskContinuationOptions.ExecuteSynchronously);
}
Можно заменить List на ConcurrentBag например, тогда не понадобятся локи.
Ну а в целом, по статье, обращусь к начинающим разработчикам: эта статья - хорошее упражнение для того, чтобы разобраться как работает этот стек под капотом, но если Вам нужно будет сделать веб-сервер для практических задач, используйте актуальную версию ASP.NET Core
Спасибо за правки, сделаю так как вы посоветовали. Извиняюсь, я ещё не настолько хорошо разбираюсь в создании ПО
Теперь про то как вы считываете данные из сокета:
byte[] data = new byte[1024];
string request = "";
client.Receive(data);
request = Encoding.UTF8.GetString(data);Никогда так не делайте!
Метод Receive не даёт никаких гарантий относительно того, сколько байт он читает из сокета!
Из-за сетевых причуд вы можете прочитать половину запроса, или два запроса подряд (это невозможно в HTTP, но возможно в общем случае).
Вы обязаны, как минимум, учитывать число считанных байт (оно возвращается из метода Receive).
Немного просмотрел код на гитхабе.
Метод GetSheet класса Client содержит код
lock (new object())
{ ... }Что по сути является бесполезным, потому что при одновременном выполнении данного кода двумя или более потоками, каждый поток будет создавать новый объект и устанавливать на нём блокировку, таким образом все потоки будут одновременно иметь доступ к критической секции. Вот пример использовании lock на MSDN.
У части классов объявлен финализатор с кодом
~ClassName()
{
GC.Collect(2, GCCollectionMode.Forced);
}Зачем?
С лок да лоханулся и забыл его убрать. Честно скажу забыл зачем я его туда запихнул, спасибо что обратили внимание.
А по второму я собирался сделать деструкторы но в итоге не совсем понял как из тут можно сделать если тут в принципе есть мусорщик, и в итоге забвл удалить. Если Вам будет не трудно то не могли бы вы привести пример чтобы в будущем я совершал меньше ошибок.
Использование финализаторов в C# - это очень редкое дело, и чаще всего используются при реализации интерфейса IDispose, для освобождения unmanaged ресурсов в случае, если объект не был корректно Dispose-нут.
В 99% случаев финализатор в C# просто не нужен - сборщик мусора всё сделает за вас. Это не C++.
Вот удивительно. То-есть этот скажем так проект, порождён в рамках учебного курса, о чём написано в самом начале. Курс 3, то-есть как бы не новичок. Ошибка на ошибке. Отсюда вопрос, чему учат на курсах? Какие специалисты работают в IT? И много ли таких скажем так специалистов выходит на рынок труда?
Чему учат в колледже:
1 курс : офисный пакет по информатике и 10-11 класс по остальным предметам
2 курс: кое как прошли архитектуру аппаратных средств, изучение C#:
Типы данных
Условные конструкции и циклы
Функции
Массивы
ООП
WinForms
Обучал нас преподаватель который работал только с аппаратной частью и последнее когда она видел код это был язык ассемблера и Си++, и было это давно. Читал я наперёд, понравилось и продолжил, и всё что наговнокодил это процесс самостоятельного обучения.
3 курс: изучаем в данный момент 1С Предприятия 8.3 ради демо экзамена, ибо площадка больше не по до что не приспособлена.
Такой ответ вас удовлетворит?
Врядли когда-либо будут учить "хорошей архитектуре". Т.к. архитектура меняется, паттерны меняются или заменяются другими и т.д.
А тут мы видим просто отсутствие опыта. Не знание, что такое Task (внутри).
У нас на факультете фундаментальный подход к изучению языков программирования: 1 курс — Паскаль и Ассеблер, 2 курс — C/C++, 3 курс — C#, 4-5 курсы — Python. По нашим меркам это совсем новичок.
Интересно, это одна из статей "на зачёт автоматом", о которых недавно модераторы Хабры писали?
Народ ну что вы набросились ? если есть предложеничя как сделать лучше - пишите реквесты в гит-хаб
Парень молодец, идеального кода не существует , есть тот который работает в заданных условиях.
И не можете помочь сами - хоть покажите на идеальные аналогичные проекты с вашей точки зрения
Спасибо за поддержку. Но критика уместна, так как имеются косяки. Может быть местами как-то грубовато, но нужно научиться правильно воспринимать критику.
Для начала мы должны добавить в наш класс библиотеки, которые нам понадобятся
Директива using не добавляет библиотеки, а лишь позволяет использовать короткие имена классов вместо полных, например
using System;
DateTime d = DateTime.Now;вместо
System.DateTime d = System.DateTime.Now;А добавление внешних библиотек - в файле прокта csproj, например через добавление PackageReference с нужной ссылкой (в листинге пример нового sdk-style типа csproj):
<Project Sdk="Microsoft.NET.Sdk.Web">
<PropertyGroup>
<TargetFramework>net5.0</TargetFramework>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="NLog.Web.AspNetCore" Version="4.14.0" />
</ItemGroup>
</Project>
Такое ощущение, что все что можно было сделать не так, было сделано не так, да еще и с особым упорством. Ощущение, что расчехлили какой-то античный фреймворк 3.5 и C# 4 (или какие там версии соответствуют друг другу), до тасок и асинков, и упорно пытались написать максимально неподдерживаемый и нечитаемый код.
Я даже не уверен что в этом виноват автор. Мне кажется это качество преподавания в вузе. Если так и есть, вот вам совет от незнакомца в интернетах: читайте книги, блогпосты, смотрите доклады с конференций, участвуйте в конференциях, следите за изменениями в языке и платформе, за best practices, и не бойтесь декомпилировать BCL что разобраться, что там под капотом. И если вы будете открывать свой код месячной давности и ваша первая мысль будет "что за косороукий долбоеб это написал? А, это же ьыл я" -- значит вы идете в правильном направлении.
Я обучаюсь не в вузе, а колледже на СПО 09.02.07 "Информационные системы и программирование". Как я уже писал одному из комментаторов, C# нас особо не учили, и тот говнокод что есть, был изучен самостоятельно. Так что получается что тут моя вина
C# нас особо не учили
Так что получается что тут моя вина
И да, и нет. Я понимаю что ждать от образовательных учереждений обучения актуальным знаниям и навыкам (особенно в РФ) -- бессмысленно, но все же хочется, чтобы человек, отучившийся на специальности, получал навыки, которые ему потом пригодятся.
Так вот, ваш самый ценный навык -- это способность искать информацию и обучаться. Вам зачем нужен был веб-сервер? Показать, что вы можете написать N строк кода? Или выдавать какой-то контент по запросу? В перпом случае бросаться на написание своего сервера с нуля -- такая себе задача. Во втором случае -- добро пожаловать в ASP.NET minimal APIs.
Репозиторий на гитхаб выглядит страшно, а ридми написан на советском языке, что сразу указывает на то, что единственная цель -- "собрать классы" зачеты. И опять же, если ваш преподаватель открыл это и сказал "Да, отличный проект, и документация просто топ, два чая этому господину" -- то преподаватель виноват не меньше вас.
Кроме того, у вас в репозитории лежат бинарники интерпретаторов и хрен пойми чего. Вы уверены, что имеете право их распространять? У вас лежит пустой .gitignore. и закоммитчены объектные файлы и бинарники приложения.
Хотите прокачать навык? Разберитесь с gitignore и вычистите репозиторий (спойлер -- у dotnet cli есть встроенный темплейт под эту задачу). Возьмите нормальный тестовый фреймворк и перепишите на него тесты, чтобы можно было спокойно вызывать dotnet test. Разберитесь с GitHubActions и добавьте пайплайн, который хотя бы будет срабатывать на пуш в мастер -- собирать проект и тестировать. Добавьте лицензию (этому должны учить до того как допускать к написанию кода) -- никто не захочет связываться с вашим проектом если у него мутная лицензия или ее нет. Напишите описание на английском (хотя бы короткое). И -- voilà -- ваш проект похож на проект. А исправлять ужасно написанный код вы теперь сможете хоть с тетриса сидя в уборной -- CI пайплайны выполнят за вас всю работу по сборке и тестированию.
Хотите быть более стильным и молодежным? Прочитайте про структурное логирование, заведите логгер и замените все Console.WriteLine на различные виды логирования (см. Microsoft.Extensions.Logging.Abstractions, если не ошибаюсь, для стандартного интерфейса). Ах да, выше уже говорили -- уберите финализаторы с принудительным GC, это просто зло в квадрате. А если вас этому кто-то обучил, то скажите этому человеку, что он не прав, и пусть лушче оставляет свои костыли в ламповом C++ -- в C# нам такое не нужно. А еще прочитайте про рекомендованный код-стиль и нейминг-конвеншн от Майкософт -- большая часть кода на C# ему следует, и нет никакой разумной причины не привести свои проекты в соответствии со стандартом.
Что в итоге? В итоге у вас останется все еще чудовищно написанный сервер, но вы получите полезные навыки работы с инфраструктурой и немного новых знаний о best practices в C#. Если вы не хотите работать за плошку риса на гос контору, где качество кода примерно соответствует вашему проекту, а версионирование происходит в архивах с суффиксом _final_final.rar, то все описанные выше навыки (и еще много чего другого) вам обязательно пригодятся.
Да ладно вам, парни, все когда-то с чего-то начинали, в т. ч. и с подобных велосипедов с квадратными колесами ))
"Не стреляйте в пианиста, он играет как умеет" ©
P. S. Автор, вот образец файла gitignore под Visual Studio и C#, в частности:
https://github.com/github/gitignore/blob/main/VisualStudio.gitignore
Для реализации многопоточности достаточно было использовать BeginAccept вместо Accept. Для реализации многопоточного вебсервера - HttpListener вместо socket.
А писать на сокетах в .net свой вебсервер имеет смысл если [есть много времени]/[отсутствуют доступные классы]/[функционала доступных недостаточно].
Благодарю, учту на будущее
Для реализации многопоточности достаточно было использовать BeginAccept вместо Accept
Это устаревший и неудобный функционал. Сейчас в тренде таски и async-await парадигма.
А писать на сокетах в .net свой вебсервер имеет смысл если [есть много
времени]/[отсутствуют доступные классы]/[функционала доступных
недостаточно].
Самый лучший способ разобраться, как работает технология — это написать велосипед и понабивать на нём шишек. Так что, несмотря на низкий уровень самой статьи, я целиком и полностью поддерживаю автора и готов ему помогать.
Благодарю за поддержку
Функция асинхронного вызова с коллбеком в отдельном потоке "из коробки" на 100% удовлетворяющий условию задачи - "старый и неудобный функционал"?! Что же это за тренды такие, где изобретать велосипед, но следуя парадигме, важнее самого велосипеда ))
Но по поводу шишек соглашусь. Я это упомянул - если хотя бы одно из условий присутствует
Потому что колбэки — это лапшеподобный код + отсутствие возможности использовать CancellationToken. К слову, сейчас под капотом BeginAccept — это враппер над AcceptAsync.
"лапшеподобный код" в контексте Begin** операций для сокет-объектов - это какой стереотип ради парадигмы, уж простите. А CancelationToken - опять же шашечки или ехать? Если первое - то наверное это серьезный аргумент. А второе - вызовите EndAccept и обработайте исключение.
Я не против трендов. Я за парадигмы. Но прежде всего - я за здравый смысл. А в рекомендации использоваться BeginAccept он заключался в том, что это самый простой, самый стабильный способ получить функционал неблокирующего многопоточного обработчика входящих соединений. Если функционала не хватает - другой разговор. Но в обсуждаемой задаче его как раз "достаточно"
Условию какой такой задачи эта функция удовлетворяет? Напомню, что нормальный сервер должен вызывать операцию Accept в цикле. Ну и как будет выглядеть цикл на BeginAccept? Как-то так:
void OnAccept(IAsyncResult ar) {
var client = socket.EndAccept(ar);
socket.BeginAccept(OnAccept, null);
Process(client);
}А вот так выглядит цикл на задачах:
while(...) {
var client = await AcceptAsync();
Task.Run(() => Process(client));
}Ну и нахрена в 2021м году писать цикл, который не выглядит как цикл?
ненене, это вы что то свое придумали )
void OnAccept(IAsyncResult ar) {
var client = socket.EndAccept(ar);
Process(client);
}
Выше - вот так будет выглядет ваш коллбек.
А вот так - обработчик входящих соединений
while (true)
{
socket.BeginAccept(new AsyncCallback(OnAccept), socket);
}PS - Извините, я еще с редакторам не разобрался. Но так или иначе - вполне себе цикл, правда )
Вот вы и накосячили с циклом. А ещё говорили что-то про "стабильность"… Вы правда не видите в этом коде ошибки?
Тогда подскажу: поскольку блокирующей операции у вас в цикле нет — вы в цикле начнёте неограниченное число операций Accept пока не закончится память или другой ресурс.
Накосячил. Но цикла это не отменяет. Я хотел продемонстрировать наличие цикла, а не строки кода.
Я пишу на vb.net и у меня сами строки выглядит так
While True
Dim Result = Socket.BeginAccept(New AsyncCallback(AddressOf Accept), Socket)
Result.AsyncWaitHandle.WaitOne(AcceptTimeout)
If Not Result.IsCompleted Then Socket.EndAccept(Result)
End While
Но повторюсь - вполне себе цикл
Ну классно. Теперь у вас и вовсе блокирующий вызов замаскированный написан. И нахрена тут вообще асинхронность-то?
Вы вообще в курсе что обращение к AsyncWaitHandle создаёт этот самый WaitHandle, который при нормальном сценарии никогда не создаётся?
Или что Socket.EndAccept(Result) ожидает завершения операции Accept, и ваш тайм-аут в итоге вообще ничего не делает?
Блокирующий - waitone? Да, но он никуда не маскируется - он находится на своем месте там как раз для того, чтобы исключение недостатка памяти не было выброшено. Можно было бы использоваться какой то другой блокирующий объект - но для чего.
Что касается таймаута - он невостребован только практически. В теории, если что-либо не позволит вызвать EndAccept в коллбеке - он сократит блокирование основного потока на время таймаута, а вызов EndAccept инцициирует вызов колбека, в котором EndAccept вызовет обратываемое исключением..
Поэтому я не вижу повода для "нахрена тут ассинхронность"
И да, вы как то меняете "вектор атаки" ) Во первых - мы заговорили не об ассинхронности, а о многопоточности. Ассинхронный вызов - способ ее реализации, который я рекомендовал в качестве самого простого. Во вторых - "нормальный сервер должен вызывать операцию Accept в цикле" - и да. основной поток суть есть цикл в котором происходит ассинхронный вызов терминирования соединения. Вы отстаиваете свой вариант - я понимаю. Но я не увидел аргументов ЗА. Я только увидел непонятные ПРОТИВ ))
А зачем вы вообще используете BeginAccept если вы тут же блокируете поток? В чем плюс асинхронного вызова, если вы по факту не используете здесь асинронность? Чтобы показать, что мы и так умеем?
По какому факту она тут, простите, не используется? ))
Основной поток блокируется в цикле ожидании первого входящего соединения. Нет соединения - что ему еще делать?
Как только оно будет терминировано, срабатывает коллбек-обработчик, который сигнализирует основному потоку, блокировка снимается, снова вызывается BeginAccept и поток снова блокируется в ожидании входящего соединения.
То что вы вызываете асинхронный метод, а затем блокируете поток до того момента, когда он завершиться, не делает ваш код асинхронным. С тем же успехом можно вызывать синхронный Accept -- результат и поведение кода будут тем же самым.
Я вызываю ассинхронный метод как самый простой, самый быстрый способ передать обработчик входящего соединения в отдельный поток и освободить основной поток для обработки ожидания следующих соединений.
Вызов синхронного метода Accept отдаляет момент передачи обработки как минимум на один шаг. А в реальности больше - ведь при терминировании соединения может произойти исключение - и вам его обрабатывать в основном потоке, задерживая очередь.
Стоп. А с чего вы взяли что блокировка основного потока ожидает ЗАВЕРШЕНИЯ коллбека? Блокировка ожидает сигнала из коллбека. EndAccept в первой же строке коллбека инициирует его а дальше спокойно в отдельном потоке обрабатывает соединение
Самый простой способ — это Accept + Task.Run. Никакой задержки очереди здесь нет и в помине, потому что вы не понимаете, как работает BeginAccept под капотом.
Я заглянул. Загляните и вы.
https://github.com/microsoft/referencesource/blob/master/System/net/System/Net/Sockets/Socket.cs
Сможете со ссылкой на "под капотом" прокомментировать отсутствие задержок? Я не готов, используя работающие функции из коробки, изучать "как это сделано".
И вы правда со вчерашнего вечера убеждаете что это
var Result = Socket.BeginAccept(new AsyncCallback(Accept), Socket);
Result.AsyncWaitHandle.WaitOne();
проще чем это?
var Client = Socket.Accept;
Task.Run(() => Accept(Client));
Наверное это и правда имеет значение. Но не для меня
Начнём с того, что вы привели код из .NET Framework 4.8. Актуальный код находится здесь:
Посмотрите, какое дикое количество кода выполняется при BeginAccept, особенно в .NET Framework, тогда как при Accept — небольшой блок проверок и сразу переход к системному вызову.
К слову, задержки происходят при постоянном перекидывании управления с одного потока на другой, а при синхронном вызове этих задержек нет.
P.S. А что вы вообще под задержкой имеете в виду?
По вашей ссылке паттерн Begin/End реализован через TaskToApm класс - так что все сведено к тем же таскам.
/// <summary>
/// Provides support for efficiently using Tasks to implement the APM (Begin/End) pattern.
/// </summary>
internal static class TaskToApmНо мы правда с вами углубились в какие-то странные сравнения через "количество" кода )) Только вы сравнивая Accept и BeginAccept не учли, что в вашем варианте "дикое количество кода" находится в Task.Run.
Переключения - их количество одинаково. 1 поток ожидания + переключение на поток обработчика.
По поводу задержки - в случае выброшенного исключения при входящем подключении при использовании Accept вы их все будете обрабатывать в основном потоке. В случае с ассинхронным вызовом - только те исключения, которые связаны с невозможность слушающего сокета принять входящее подключение. Но тогда уже и на очередь "по барабану"
PS - Вообще это уже не спор, а холивар какой то. Я запустил простой цикл который подключается и отключается к обработчику ожидания как через Task так и через Begin. Разница на 10000 подключений - 74мс. То есть прикладного смысла в споре нет. А идейный - считайте меня ортодоксом )))
Переключения - их количество одинаково. 1 поток ожидания + переключение на поток обработчика.
Таки нет. В вашем случае переключений намного больше: выполнение операции и получение её результата происходит в отдельных потоках. И дальше ещё 2 переключения: возврат управления основному циклу через WaitHandle и вызов AsyncCallback в пуле потоков. Так что не забываем про бритву Оккама и используем синхронные операции там, где нет смысла в асинхронных.
По поводу задержки - в случае выброшенного исключения при входящем подключении при использовании Accept вы их все будете обрабатывать в основном потоке.
Эти же исключения вы поймаете при EndAccept.
Вообще это уже не спор, а холивар какой то. Я запустил простой цикл который подключается и отключается к обработчику ожидания как через Task так и через Begin. Разница на 10000 подключений - 74мс
Так и есть. Но вы первый начали, развязав обсуждение про мифическую задержку очереди.
Это не так. Переключений ровно столько же. Я же давал ссылку на "устаревший" 4.8. Помотрите. Найдите подтверждение своим словам. Но если лень - давайте "от обратного". Вы утверждаете что в варианте Begin*** переключений в три раза больше? Допустим. В таком случае эмпирическим путем установлено, что "цена" одного переключения в случайно взятой (моей) тестовой среде - 74/((3-1)*10000) мс
или 0,0037мс
Вас это значение заботит?! Меня - НЕТ ))Конечно. Только EndAccept выполняется в отдельном потоке обработчике, а не в основном потоке ожидания.
Неправда. Обсуждение начали вы, сказав что вариант с Begin*** - это "лапшеподобный и устаревший функционал" ))
Но правда - давайте закончим. Не знаю как вы, но я из нашей беседы вынес что менять в существующем коде мне ничего не нужно, что минуса на хабре растут быстрее чем грибы после дождя и то, что я не "в тренде" без утери прикладного значения )))
Очень полезные выводы
А вот с таймаутом соглашусь - лишние ложные срабатывания. Убрал у себя, оставив WaitOne()
Создание многопоточного сервера на C#