Serwer WebSocket w C#

Napisałem w C# serwer WebSocketowy, póki co obsługujący tylko sam protokół ws (bez wss). Starałem się go pisać tak, żeby tylko w przyzwoitych ramach łamał SRP (:P), więc gdyby ktoś miał czas zerknąć i powiedzieć co można zrobić lepiej to bardzo chętnie poczytam.

https://github.com/WodorHel/WebSocket-Server

Trochę kontekstu:

• Główną klasą jest WebSocketServer, który odpowiada za komunikację z użytkownikiem.
• Operacjami typowo sieciowymi zajmuje się klasa ServerListener, która obsługuje metody BeginReceive, EndReceive i całą resztę.
• Obsługą Handshakeu zajmuje się Protocol/Handshake/HandshakeResponseGenerator
• Rzeczy związane z serializacją/deserializacją ramki WebSocketowej są w Protocol/Frame

Z rzeczy które mi nie dają spokoju:

• W klasie WebSocketServer znajduje się metoda ProcessMessage która w zależności od typu ramki wykonuje odpowiednie operacje. Chętnie zmieniłbym to na jakąś fabrykę i po prostu wywoływał w jakimś FrameExecutor jedną metodę. Z tym że muszę wtedy jej przekazać cały obiekt WebSocketServer, ponieważ tylko stąd można wywołać odpowiedni event lub wysłać framkę Pong. Nie wiem czy to dobre rozwiązanie.
• Długo się zastanawiałem czy informacje o kliencie między warstwą WebSocketów (WebSocketServer), a warstwą sieciową (ServerListener) przekazywać za pomocą id (String clientID), czy za pomocą całego obiektu. Na ten moment jest to pierwsze, ale dalej mam wątpliwości.

PS. Przy okazji pragnę podziękować autorom Coyota za zapisywanie treści posta po wyjściu ze strony, właśnie uratowało mi to całą treść.
PS2. A jednak nie zapisało mi tagów, ale i tak dziękuję za uratowanie 5 minut życia.

Tak popatrzyłem, nie masz ani pliku csproj, ani sln

Dzięki, nie wrzucałem ich bo to jest konto specjalnie na cel 4p (kto normalny ma nick WodorHel lol :P) i nie widziałem za bardzo sensu. A wątpię by ktokolwiek chciał to ściągać i uruchamiać.

Ogólnie, imho dobrze, że używasz var,

Nie trzymasz się konwencji, czasem masz this, czasem var - czasem nie.

https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/SendingDataService.cs

return w catchu? Widziałem kilka sytuacji jak trzeba było tak zrobić, ale ogólnie ja się trzymam przy single return - jeżeli nie ma jakichś sytuacji wyjątkowych to return imho to taki spimpowany goto

Exception jako typ catcha? Wszystkie wyjątki traktujesz tak samo. (nie sprawdzałęm co te metody rzucają, jeżeli exceptiona z jakimś message jak czasami html agility pack to użyj filter). Typ exceptiona w catch powinien być jak najbardziej sprecyzowany - nie chodzi Ci przecież o to, by programista/user nie widział exceptionów, ale żeby były dobrze obsłużone.

Puste, domyślne konstruktory. Po co?

<przekreslenie ktorego nie wiem jak zrobić> Widziałem, że klilka rzeczy rzutujesz na objecta, po co? </...>
Ok, chyba zakładka nie z tego repo :D

Ale ogólnie widziałem gorszy kod, przynajmniej nie masz nic w stylu throw new Exception("PROBLEM").

Dziękuję serdecznie, więc po kolei:

1. Rzeczywiście w kilku miejscach pominąłem this, ogólnie stosuję go w konstruktorze bardziej ze względów estetycznych, bo sporo linijek mogłoby się bez niego obejść. Anyway, przejrzę pliki i dopiszę gdzie zapomniałem.

2. Zgadzam się, problem w tym że nie zawsze wiem jak to inaczej rozwiązać. Weźmy na przykład ostatnią metodę w https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ConnectingService.cs , jeżeli nie zrobię returna to wywołam Connected którego nie chcę. Alternatywa jaką widzę to jakiś bool failure przestawiany w catchu, i potem sprawdzany przed wywołaniem Connected.

3. Generalnie tak, dlatego rozwiązałem to w ten sposób, że exceptiona przekazuję jako argument który potem użytkownik może sobie odpowiednio interpretować. Przykładowo EndAccept ma 6 możliwych exceptionów, więc uznałem że wszystkie będę tak czy siak przekazywał jako argument, to walnąłem prymitywnego do bólu Exception :P

4. Kwestia przyzwyczajenia i szeroko pojętej konwencji :P

5. Na objecta? Z tego co pamiętam to jedyne objecty są w eventach jako sendery, a to jest chyba norma.

2. możesz dać Connected po tamtej linijce, i tak się nie wywoła.

W sumie tak, masz rację, nie wiem czemu nie pomyślałem o tym :|

Ale serio catch (Exception to zły pomysł, nawet jeżeli chcesz mieć wyjątki opakowane w delegata, to ogranicz w filterze do tych https://msdn.microsoft.com/en-ie/library/chfa7866(v=vs.110).aspx, skąd wiesz, czy ktoś nie zrobi hooka na clr i nie podmieni implementacji i będą rzucane wyjątki których nikt nie zaplanował? Poza tym zjadasz wyjątki i trudniej to debuggować bo nie ma wsparcia ide.

W sumie trochę przemyślałem to, i stwierdziłem że faktycznie lepiej to doprecyzować. Bo jak dostanę NotSupportedException (nieobsługiwany system) albo ArgumentException (brak wywołania BeginCoś), to nie jest to normalna/przewidziana sytuacja i biblioteka tak czy siak nie będzie działać. Więc user powinien o tym wiedzieć bardziej dosadnie niż przez jakiś tam bieda-event który dotyczy sytuacji stricte połączonych klientów.

1. Nazwy zmiennych niezgodne z zaleceniami C#.
2. Wyjątek BufferOverflowException nie ma wszystkich zalecanych konstruktorów, do tego nie jest Serializable.
3. String zamiast string w wielu miejscach.
4. https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketClients/WebSocketClientsManager.cs#L11 konkretna klasa zamiast interfejsu.
5. https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ConnectingService.cs#L49 To powinno być w try i wtedy nie musisz robić return w catchu.
6. https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ReceivingDataService.cs#L31 To wygląda bardzo podejrzanie, dlaczego po prostu zjadasz wyjątek?
7. https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ReceivingDataService.cs#L61 To też do bloku try.
8. https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/SendingDataService.cs#L32 A ten return już w ogóle nic nie robi, kopiowałeś kod?
9. Puste konstruktory.
   To tak tylko po sprawdzeniu składni, w logikę się nie zagłębiałem.

1. Tj. o jakie przykłady konkretnie chodzi? Przypuszczam że o nazewnictwo eventów albo EventArgsów, ale upewnij mnie.
2. Właśnie doczytałem dlaczego powinien być Serializable, pójdzie do poprawy :P
3. W sumie całe życie używałem String zamiast string, i z tego co wiem to nie ma znaczenia który bo pierwszy jest aliasem drugiego. Podejrzewam że to też kwestia jakiejś konwencji? EDIT: Jeszcze doczytałem i z tego co widziałem to faktycznie string (tak samo jak int a nie Int32 itp.) jest preferowany. Cenna wskazówka :P
4. Do poprawy.
5. 6, 7, 8 Poprawione po wcześniejszym poście, returnów w try-catch już nie ma, a wyjątki obsługuję tylko konkretnych typów, reszta leci do usera.

9 No i znowu puste konstruktory, wiem że KISS ale klasa bez konstruktora wygląda strasznie nijako :P

Dziękuję bardzo, kilka punktów jest bardzo cennych bo nigdy o nich nie myślałem w sumie, a są logiczne :P

1. Chociażby o nazwy zmiennych prywatnych, one powinny mieć podkreślenie.

Podkreślenie? W każdym artykule na temat notacji widzę zalecenie by nie używać tego spadku po notacji węgierskiej w C#, co jest opisane przez Microsoft chociażby tutaj:

Do not use Hungarian notation
Do not use a prefix for member variables (, m, s_, etc.). If you want to distinguish between local and member variables you should use “this.” in C# and “Me.” in VB.NET.
Do use camelCasing for member variables
Do use camelCasing for parameters
Do use camelCasing for local variables
Do use PascalCasing for function, property, event, and class names
Do prefix interfaces names with “I”
Do not prefix enums, classes, or delegates with any letter

https://blogs.msdn.microsoft.com/brada/2005/01/26/internal-coding-guidelines/

Lub tutaj:

X DO NOT use underscores, hyphens, or any other nonalphanumeric characters.
X DO NOT use Hungarian notation.

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions

Zawsze w C# zmienne prywatne zaczynały się od podkreślenia, żeby nie używać wszędzie this, tak też sugeruje R# i tak też pisze większość znanych mi programistów. No i nie wspomnę już o tym, że w VB.NET wielkość liter nie ma znaczenia i tam nie da się odróżnić property od pola w ten sposób, więc trzeba i tak użyć innej nazwy.
Nawet ludzie tworzący .NET stosują podkreślenia: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md
Co do:
X DO NOT use underscores, hyphens, or any other nonalphanumeric characters.
To jest coś innego, to wymóg dla kodu CLS compliant, a to nie obejmuje prywatnych zmiennych.

W sumie masz rację, pooglądałem sobie kilka topowych projektów na GitHubie i poza jednym wszystkie używały notacji z podkreśleniem. Zawsze wydawało mi się że podreślenie to relikt przeszłości z czasów świetności C i C++, ale jak widać byłem w błędzie :x

Wrzuciłem commita z poprawkami, mam nadzieję że niczego nie przeoczyłem. Z nowych rzeczy, dodałem fabrykę egzekutorów poleceń (lol), która trochę skróciła mi kod w głównej klasie WebSocketServer jeśli chodzi o interpretację nadchodzących ramek, jakąś metodę do pobierania informacji o połączonym kliencie i coś tam jeszcze. Link ten sam oczywiście :P

https://github.com/WodorHel/WebSocket-Server

https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ReceivingDataService.cs#L22 jeżeli if nie wejdzie zjadasz wyjątek

Przy okazji brak klamer po tym ifie.

To jest akurat efekt zamierzony. ObjectDisposedException wywołuje się gdy gdzieś w jakimś miejscu socket jest zamykany. BeginReceive (bądź jakakolwiek inna metoda z Begin na początku) może wywalić mi wtedy taki exception. Jeżeli klient ma oznaczenie "Closed", to uznaję że exception jest właśnie z powodu zamknięcia socketu i go ignoruję. A jak nie, to leci gdzieś tam dalej do usera, bo niezamknięty socket generujący ObjectDisposedException to już jest problem.

https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/ReceivingDataService.cs#L49
To zamień tego ifa na filtr z nowego C#. I od razu pytanie-zagadka: dlaczego filtr, a nie tak, jak jest teraz?

https://github.com/WodorHel/WebSocket-Server/blob/master/ServerListener.cs#L62
Wydaje mi się, że zazwyczaj endpoint traktuje się jako jedno słowo, więc p nie powinno być wielką literką.

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L123
Tu chyba nie ma sprawdzenia, czy klient istnieje.

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L182
https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L200
Te ify mogłyby być odwrócone.

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L181
Logika z tej metody i wszystkich podobnych – nie wiem, czy to nie powinno być gdzieś indziej. Jakoś mi tutaj średnio pasuje. Może w WebSocketClient?

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L54
Chyba powinien być jakiś minimalny rozmiar bufora, a nie cokolwiek.

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L25
Bufor powinien być liczbą nieujemną.

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L127
Nie ma takiego słowa jak informations.

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L150
Wydaje mi się, że drugi parametr powinien nazywać się e, tak chyba zaleca MS. Jak już tak przerzucamy się nazewniczymi hakami ;)

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L195
DecryptResult jakoś słabo dla mnie brzmi, nie lepiej DecryptionResult? DecryptingResult?

https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/SocketClients/Client.cs#L4
Tu masz złą przestrzeń nazw (potencjalnie w innych plikach też).

https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/SocketClients/Client.cs#L17
Jakoś nie jestem przekonany, że id jako guid serializowany do stringa jest dobrym pomysłem. Dlaczego nie zwykły integer?

https://github.com/WodorHel/WebSocket-Server/blob/master/SocketServices/SocketClients/Client.cs
Używasz klasy Socket, więc powinieneś implementować IDisposable.

https://github.com/WodorHel/WebSocket-Server/blob/master/ServerListener.cs#L33
Słabe mi się to wydaje, to raczej powinien być jakiś enum opisujący dokładnie stan serwera.

https://github.com/WodorHel/WebSocket-Server/blob/master/ServerListener.cs#L64
Co się stanie, jak ktoś wywoła tę metodę dwa razy? A co się stanie, jak wywoła ją, potem StopListening, a potem znowu StartListening?

A taki piękny kod był w moich oczach... :D Ale poważnie rzecz biorąc to dzięki, rzeczywiście to wszystko pójdzie do poprawy. Mam tylko problem z jednym punktem, a mianowicie:

https://github.com/WodorHel/WebSocket-Server/blob/master/WebSocketServer.cs#L181
Logika z tej metody i wszystkich podobnych – nie wiem, czy to nie powinno być gdzieś indziej. Jakoś mi tutaj średnio pasuje. Może w WebSocketClient?

Też mi pasuje średnio upychanie takich rzeczy do tej klasy, którą chętnie traktowałbym tylko jako interfejs między userem a libką (tj. same eventy + jakieś metody Send/GetInfo itp.). Z drugiej strony, WebSocketClient nie ma dostępu do metod które wysłały by handshake bądź ramki z danymi. Na szybko wymyśliłem coś w stylu klasy Session, która takie metody by miała i byłaby przekazywana do klasy WebSocketClient, dzięki czemu mogłaby sobie już sama zrobić handshake, zserializować i wysłać ramkę itp. Takie rozwiązanie widziałem chyba w innej bibliotece, ale nie znajdę teraz.

EDIT: Po głębszym przemyśleniu, takie rozwiązanie pozwoliłoby nieco uprościć budowę, bo miałbym wtedy tylko jeden kontener z połączonymi klientami, a nie dwa, tak jak jest teraz. Ale zobaczymy jak to w praktyce wyjdzie, wrzucę nowego commita jak będę miał już gotową wersję.

Afish napisał(a):

Zawsze w C# zmienne prywatne zaczynały się od podkreślenia, żeby nie używać wszędzie this, tak też sugeruje R#

No bez żartów, podkreślenia to jakaś węgierska mentalność. A R# w swoich domyślnych szablonach dodaje this.
Ja wiem, że jest wiele projektów, w których stosuje się te obleśne podkreślenia, ale to nie znaczy, że to jest jakieś "zawsze". Dobrzy programiści ich raczej nie stosują.

i tak też pisze większość znanych mi programistów.

Nie ma to jak argument znajomych z dzielni. ;)

Nawet ludzie tworzący .NET stosują podkreślenia: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

Prefix instance fields with _, static fields with s_ and thread static fields with t_

W 2017 roku. :|

somekind napisał(a):

No bez żartów, podkreślenia to jakaś węgierska mentalność. A R# w swoich domyślnych szablonach dodaje this.

To jest jeszcze coś innego, R# dodaje this, a potem go podświetla i sugeruje jego usunięcie. Ja mówię o nazwie zmiennej prywatnej, R# domyślnie sugeruje dodanie podkreślenia do nazwy.

Nie ma to jak argument znajomych z dzielni. ;)

A jakich argumentów oczekujesz w rozmowie o standardzie de facto?

R# można skonfigurować żeby nie zachowywał się śmiesznie, VS też, piszę bez R# i mam sugesite this (co prawda kulawa bo base nie uwzględnia, ale jest).
To że 'w standardzie' system.reflection.emit łamie LSP to też jest dobre?

Pixello napisał(a):

R# można skonfigurować żeby nie zachowywał się śmiesznie

Jeżeli musisz coś ręcznie zmienić, to automatycznie większość osób tego nie zrobi i będzie używało domyślnych, przez co większość kodu będzie miało podkreślenia, a w formatowaniu i nazewnictwie najważniejsza jest spójność.

Taki problem dodać do repo .dotsettings i .editorConfig żeby wymusić coding style?

Afish napisał(a):

To jest jeszcze coś innego, R# dodaje this, a potem go podświetla i sugeruje jego usunięcie. Ja mówię o nazwie zmiennej prywatnej, R# domyślnie sugeruje dodanie podkreślenia do nazwy.

Sam widzisz, że nawet twórcy ReSharpera nie potrafią trzymać się spójnie jednego podejścia.
Microsoft najwyraźniej również, skoro w jednych produktach zaleca używanie podkreślników, w innych ich zakazuje (reguły StyleCopa).

A jakich argumentów oczekujesz w rozmowie o standardzie de facto?

Ale jakim standardzie? To tylko konwencja, nie żaden standard.

A argumentów oczekuję zawsze takich samych - co to wnosi do kodu? Jedyne racjonalne wyjaśnienie, to chyba ułatwienie pracy słabym programistom, którzy piszą tak długie metody, że nie potrafią się połapać, która zmienna jest lokalna, a która jest polem i nie znają do tego this.

Afish napisał(a):

Jeżeli musisz coś ręcznie zmienić, to automatycznie większość osób tego nie zrobi i będzie używało domyślnych, przez co większość kodu będzie miało podkreślenia, a w formatowaniu i nazewnictwie najważniejsza jest spójność.

Czyli problemem są leniwi programiści, którzy nie potrafią używać własnych narzędzi?

Co do spójności się zgadzam - w jednym projekcie powinna zostać wybrana jedna konwencja i tak też powinny zostać ustawione wszelkie narzędzia, zarówno na poziomie indywidualnych programistów jak i CI. Ale to nie znaczy, że konwencje w projekcie X są mądre ani że od razu stają się standardami.

Tak, problemem są leniwi programiści i domyślne ustawienia narzędzi. Oczekujecie ode mnie, że zmienię sytuację rzeczywistą i standard de facto? Standard de jure od MS mówi, że podkreśleń być nie powinno, ale jeżeli domyślnie narzędzia sugerują coś innego, to nic dziwnego, że ludzie stosują podkreślenia bezwiednie podążając za wytycznymi R#. I nawet jeżeli da się to zmienić jednym kliknięciem, to większość tego nie zrobi.

Bawi mnie, że tak na mnie najeżdżacie, skoro to nie jest moja wina. Ludzie czytają tutoriale o MVC i potem pchają DataContext do kontrolerów, bo tak MS daje w tutorialach, podobnie jest z podkreśleniami sugerowanymi przez R# — czy te podkreślenia mają sens, czy nie mają, to jest już prywatna opinia (której też ja nigdzie nie wyraziłem), ale większość ludzi pójdzie zgodnie ze wskazówkami narzędzia.

@Afish: ale ja Cię absolutnie nie winię o sytuację. Ja tylko uważam, że nie powinieneś nazywać konwencji (nawet jeśli jest częściej stosowana) standardem.

Moim zdaniem to logomachia, ale jeżeli ma to dla Ciebie aż takie znaczenie, to mogę to nazywać konwencją.

Zwróciłeś autorowi wątku uwagę, że robi coś źle, przy czym wcale tak nie było, a robił jedynie nie po Twojemu.