Dapper - insert danych powoduje deadlock

Witam.
Mam panel w Angularze + ASP .NET Core 3.0 za pomocą którego dodaje się dokumenty. Mam możliwość dodawania dwóch typów dokumentów RO (Rezerwacja odbiorcy) oraz FPF (Faktura Pro Forma). W przypadku dodawania FPF, problemu nie ma, wszystkie pozycje są poprawnie dodane, ale w przypadku RO dostaje deadlocki... Metoda dodawania dokumentów ta sama.

Czy może mi ktoś wyjaśnić jakim cudem insert danych dapperem powoduje deadlock? Czy jestem w stanie coś z tym zrobić, jakiś timer, thread.sleep? Nie rozumiem dlaczego akurat teraz mam takie problemy.

Pokaż kod. Może masz jakiś trigger, który to powoduje.

Ok kilka wdechów i jedziemy.

Za każdym razem otwierasz asycnhronicznie połączenie do bazy (czemu ?) w InsertTraElem. Nie czakasz na wstawienie tych elementów _sqlService.InsertTraVAT(trv) nie ma awaita (napisałbym że nie ma .Result ale przez palce mi to nie przejdzie) więc problem leży tutaj.

Błagam poczytaj o kolekcjach, asynchroniczności, łapaniu wyjątków, nazwach zmiennych, łączeniu się z bazą. Tutaj ani jedna linijka nie jest zgodna ze sztuka i konwencjami w .NET

Kod jest zły, bardzo zły...

Przemyśl sobie cała strukturę bo to naprawde powinno wyglądac inaczej. Zakładam że wywołujesz to w kontrolerze - jeśli tak to nie widze powodu żeby nie ciagnąc asynychronicznych wywołan aż do kontrolera - .Result to najgrosze co można tu zrobić. Nie otwieraj poaćzenia za każdym razem w pętli for - otwieraj jedno na całą metodę.

Co mam wyczytać z tych kolekcji, asynchroniczności, łapania wyjątków, nazwach zmiennych, łączeniu z bazą? Czego mi brakuje? Dlaczego nazwy zmiennych dla ciebie są złe? Co jest złego w łapaniu wyjątków i co jest złego z kolekcjami? Ok, z połączeniem do bazy masz rację, ale to proszę o jakąś propozycję, jak mniej więcej to poprawić, co zmienić, gdzie ?

Ja jeszcze bym dodał że enigmatyczne nazywanie zmiennych nie jest "cool", serio. To praktyka stosowana 20 lat temu i dziś jeszcze przez niektórych hinduskich programistów. Zmienne powinny mieć czytelne, jasno określające ich rolę nazwy. To samo tyczy się typów. No bo czym jest TraNag dla postronnego czytelnika? Mi nic to nie mówi. Z kontekstu domyślam się że to coś związanego z transakcjami, ale kod w którym trzeba się czegoś domyślać to zły kod. Nie ma naprawdę nic złego w długiej nazwie jeśli dobrze odzwierciedla ona przeznaczenie zmiennej/typu.

trv.TrV_TrNid

To już w ogóle czarna magia. Pomijam podkreślnik w nazwie właściwości...

Dlaczego zakładasz, że nic sensownego z nim nie robię. Właśnie dzięki temu wyjątkowi wiem, że mam błąd w dodawaniu do bazy i deadlocki. Mój middleware rzuca błędem ale to nie jest kwestia tego wątku i ten błąd nic mi nie mówił. Klient zgłasza, że podczas testów dodaje się tylko jedna pozycja do dokumentu więc drążę temat. Nie mam zamiaru tracić czasu i poprawiać go tylko po to, aby poprawnie wrzucić na forum.

Jedni twierdzą, że wszystko co związane z SQL ma być w osobnym serwisie, ty twierdzisz, że obiekt połączenia mam tworzyć w poszczególnych serwisach i w parametrach go przekazywać. Każdy piszę inaczej, a ja trace czas i się próbuje do "każdego" dostosować. Problem jest taki, że mam deadlocki, a nie "brzydki" kod.

Problem jest taki, że mam deadlocki, a nie "brzydki" kod.

To nie jest brzydki kod, to jest zły kod, który powoduje deadlocki.
A co robisz z tym wyjątkiem ? W jakis sposób go obsługujesz, próbujesz załagodzić jego skutki ?

Czekaj czekaj ? To jest kod produkcyyjny ? Ktoś tego używa ?

Tak, to jest kod produkcyjny, ktoś tego używa. Teraz właśnie próbuję coś zrobić z tym co zwrócił mi wyjątek

Ok, dyskusja nie ma sensu. Odpowiedź uzyskałes. Deadlocki wynikają z całkiwicie błednej obsługi asynchroniczności, która wynika z fatalnej strukury kodu. Jesli chcesz to naprawic na już (dopchąnac butem i zakleić taśmą) to linijka 10 i 18 powinny na koncu mieć .Result.

Powodzenia Tobie i (przede wszystkim) klientowi.

Ja się nie zamykam. Daj jakieś przykłady z gita jak to powinno wyglądać. Jeśli robię coś źle to to chce poprawić. Ale pisanie mi tekstu typu "poczytaj sobie" na mnie nie działa, ponieważ czytanie mnie niczego nie uczy, a już tym bardziej dokumentacji MS.

Chce pisać dobrze, ale niekoniecznie trzeba mnie w ten sposób dissowac.

Sugerując się postami jakie tutaj wpadają od ludzi, którzy chcą wiedzieć czy dobrze piszą, widzę, że im więcej ludzi tym więcej zdań, więc pytanie moje - kto ma rację?

Struktura tego fragmentu duzo mówi na temat organizacji całej aplikacji. Żeby tutaj cos sensownie poprawić musiałbyś pokazać cały kod - całe repo. To nje jest jeden fix w jednym punkcie.
1 . Asynchronicznośc to jest njapoważniejszy problem (BTW? skąd w ogoóle w tej metodzie wział ci sie async ? ).
2. Jesli to ASp.NET to połaczenie powinno być jedno per request - wstrzykiwane do serwisu z poziomu middleware. To nie wyklucza zmaknięcia całego SQLa w serwisie - jesli zrobisz poprawne Dependency Injection
3. Serwis nie powinien wywoływać innych serwisów
4. Kolekcje - tu naprawde musisz poczytać - to w jaki sposób dodajesz elementy to metody sprzed 20 lat, po to jest IEnumerable, Linq, foreach żeby nie iterować po elementach tablicy.
5. Wyjatki - tak jak pisałem wczesniej - gdybyś w catchu próbował jakoś zareagować na sytuację - odtworzyc połączenie zmienić dane to spoko miałoby to sens ale zalogowanie błędu nie jest jego obsłużeniem - powinien poleciec wyżej do middlewara i pokzać się userowi.

To zakleszczenie jest na poziomie MSSQL czy współbieżności w kodzie .NET?
Bo myślałem, że na poziomie MSSQL, ale teraz to już nie jestem pewien...

Bo jeśli jest na poziomie MSSQL, to... chyba wszyscy piszący mają blade pojęcie o MSSQL, czym jest deadlock w MSSQL, jak go łapać i jak się przed nim chronić.
To jak to jest @AdamWox ?

Bo ten komentarz @W2K w kontekście bazy danych MS SQL (tylko i wyłącznie kontekście bazy danych):

"Otwiera się wiele połączen (każde po kolei) ale na tyle szybko w stosunku do czasu wykonania i odpowiedzi z bazy że są one prawie jak równoległe."

To nie jest ten problem.
Pewnie, że ilość połączeń do bazy powinna być jak najmniejsza a transakcje bazodanowe jak najkrótsze, ale to zupełnie inny obszar.

" Wszytstkie odnosza się do tej samej tabeli - to zawsze rodzi problemy."

A to z kolei jest po prostu nieprawdą, jeśli w tym zdaniu "tabela " oznacza tabelę w bazie danych.
Jak wam się wydaje, skoro to prawda to jakim cudem bazy danych są wa stanie obsłużyć wielu użytkowników, którzy dodają/czytają dane z tych samych tabel?
Czy to rodzi problemy? Właśnie...

Samo dodanie wiersza do tej samej tabeli z wielu wątków, gdzie każdy z nich ma własne połączenie do bazy danych, nie będzie powodował problemów. Nigdy, jeśli zrobi się to zgodnie ze sztuką.
Ze współbieżnością w bazach danych (tak ogólnie) jest jedna złota zasada: jeden wątek = jedno połączenie do bazy danych.
Wątki nie mogą współdzielić tego samego połączenia do bazy danych w tym samym czasie w celu wykonywania operacji na bazie danych.

Rozwiązanie jakie zaproponował "na szybko" @W2K pomogło. W tym momencie dokumenty dodają się poprawnie. W dalszym ciągu nie wiem na czym polega problem, ponieważ dokumenty innego typu dodawały się poprawnie bez tej poprawki. Baza nie jest moja, to Comarch Optima, tam jest mnóstwo triggerów i "głupot" porobionych przez samego producenta.

Bo jeśli jest na poziomie MSSQL, to... chyba wszyscy piszący mają blade pojęcie o MSSQL, czym jest deadlock w MSSQL, jak go łapać i jak się przed nim chronić.
To jak to jest @AdamWox ?

Ty mi powiedz, bo ja, serio, nie mam zielonego pojęcia, a już tym bardziej jak wkroczyłeś do akcji. Czyli co @W2K ma rację, ponieważ zaczęło mi to działać, ale nie ma racji bo wyszło fartem?

AdamWox napisał(a):

Rozwiązanie jakie zaproponował "na szybko" @W2K pomogło. W tym momencie dokumenty dodają się poprawnie. W dalszym ciągu nie wiem na czym polega problem, ponieważ dokumenty innego typu dodawały się poprawnie bez tej poprawki. Baza nie jest moja, to Comarch Optima, tam jest mnóstwo triggerów i "głupot" porobionych przez samego producenta.

Tak, miałem wątpliwą przyjemność popatrzenia sobie z bardzo bliska na Comarch ERP XL przy niebanalnym obciążeniu.
Za to co zrobił producent, ale przede wszystkim wysoko opłacani konsultanci od siedmiu boleści nadaje się tylko wpierdol w dyby i do lochu.
Ci ludzie nie mają większego pojęcia o tym co tak naprawdę robią - związać drutem, dopchnąć kolanem i dalej, następny do golenia.
A i przy okazji wrogów sobie narobiłem, ponieważ na trójstronnym spotkaniu szef działu R&D dużego partnera Comarch stwierdził, że "zmiana connection string wymaga rekompilacji ich dodatku do Comarch ERP XL i będzie to kosztowało jakieś kilka tyś.".
I w tym momencie nie opanowałem odruchów i parsknąłem opluwając się kawą...

Bo jeśli jest na poziomie MSSQL, to... chyba wszyscy piszący mają blade pojęcie o MSSQL, czym jest deadlock w MSSQL, jak go łapać i jak się przed nim chronić.
To jak to jest @AdamWox ?

Ty mi powiedz, bo ja, serio, nie mam zielonego pojęcia, a już tym bardziej jak wkroczyłeś do akcji.

Zaraz, chwila...
Nie wiesz jaki masz wyjątek i co go wywołało?
Napisałeś deadlock w kontekście bazy danych.

Ja to rozumiem jako zakleszczenie po stronie MSSQL.
MSSQL w przypadku zakleszczenia zwraca odpowiedni wyjątek, o taki:
Transaction (Process ID XYZ) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.

To gdzie masz zakleszczenie, w bazie danych czy w aplikacji?

Czyli co @W2K ma rację, ponieważ zaczęło mi to działać, ale nie ma racji bo wyszło fartem?

Może tak być, ale to nie jest takie oczywiste.
Zakleszczenia w aplikacji i w bazie danych bywają dość wredne i trudne do ubicia.
To co opisał @W2K ma zastosowanie przede wszystkim do kodu aplikacji i, tak naprawdę, nie wychodzi poza obszar dobrych praktyk. A jeśli się mylę, to pewnie @W2K mnie poprawi a ja się chętnie czegoś nauczę. Nie ukrywam, że .NET to nie moje "środowisko naturalne".

Ja pisałem tylko i wyłącznie o bazie danych.

Jedno z drugim może być powiązane, ale nie musi.
Zatem skąd przyszedł wyjątek?

W skrócie : ten kod jest tak zły że aż ciekawy jako badanie zachowania bazy i połączenia :-D

Kluczowe żeby wyjaśnić co tak naprawdę się stało to konkretny wyjątek który otrzymujesz.
@wloochacz tak - zgadzam się z Tobą że bazy są zaprojektowane do tego żeby otwierać dziesiątki - setki połączeń jednocześnie i wykonywać te same operacjie. Też mnie to mocno zastanowiło. Stąd moje nieprecyzyjne określenie - to zawsze rodzi problemy. Nie jestrem pewny co dokładnie dzieje się na bazie w takiej syuacji, ale pewnym jest że otworzenie kilkudziesięciu połączeń w pętli każde tylko po to żeby wstawić jeden rekord musi skończyc się źle. Zresztą mamy bazę działającą pewnie na jakimś niespecjalnie wydajnym serwerze, a mówimy to o odpaleniu kilki do kilkudziesięciu połączeń w przedziale poniżej 1ms. Problem z pewnością leży w równoległym otwieraniu tych połączeń (nie jest wiadome czy każde z nich to osobny wątek) bo dodanie Result - czyli wymuszenie synchroniczności naprawiło błąd. I podkreślam w dalszym ciągu jest to rozwiązanie złe- nawet bardzo złe nawet po łatce którą zaproponowałem.
Sam chętnie poznam treść wyjątku jaki leci bo żeby było ciekawiej insert masz w usingu. co w ogóle przenoosi problem w jeszcze ciekawsze obszary. Istnieje szansa że połączenie ulega zamknięciu zanim się wykona całkowicie.

AdamWox napisał(a):

Rozwiązanie jakie zaproponował "na szybko" @W2K pomogło. W tym momencie dokumenty dodają się poprawnie. W dalszym ciągu nie wiem na czym polega problem, ponieważ dokumenty innego typu dodawały się poprawnie bez tej poprawki.

Może miałeś szczęście, takie rzeczy się zdarzają.

Problem polegał na tym, że napisałeś kod, który powodował deadlock - tak by design.

1. Z głównego wątku w metodzie SaveTraNag wywołujesz asynchroniczną metodę InsertTraNag.
2. W niej masz return await db.InsertAsync(cośtam);, ta linijka rozpoczyna komunikację z bazą i zwraca rozpoczęty Task (wykonywany przez jakiś wątek z puli). Task to nie jest wynik działania metody, który nas docelowo interesuje, to tylko informacja, że coś się dzieje i na to czekamy; obietnica, że działanie za jakiś czas się zakończy.
3. InsertTraNag czeka (await) na ten Task, czyli przechwytuje kontekst wywołania, aby móc kontynuować po zakończeniu Taska. Nie masz tam co prawda więcej kodu, który by na wyniku Taska działał, ale nadal to działa w ten sposób. Ten niezakończony Task zwracany jest to metody wyżej.
4. Metoda SaveTraNag z powodu użycia .Result blokuje się na tym Tasku, a więc blokuje w ten sposób wątek kontekstu.
5. Baza zakończyła pracę, a więc Task zwrócony przez InsertAsync jest zakończony.
6. Teraz InsertTraNag jest już gotowa do kontynuowania działania, więc czeka na wątek kontekstu, aby mogła iść dalej.
7. Ale kontekst jest przecież zablokowany, bo synchronicznie czeka na zakończenie InsertTraNag.
8. No i dupa... znaczy deadlock. Dwie metody w dwóch wątkach czekają na siebie wzajemnie.

wloochacz napisał(a):

Ja to rozumiem jako zakleszczenie po stronie MSSQL.
MSSQL w przypadku zakleszczenia zwraca odpowiedni wyjątek, o taki:
Transaction (Process ID XYZ) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.

@wloochacz: Ja na bazach jakoś wybitnie się nie znam, ale zakleszczenie na samych insertach? To jest w ogóle możliwe?

W2K napisał(a):

Nie jestrem pewny co dokładnie dzieje się na bazie w takiej syuacji, ale pewnym jest że otworzenie kilkudziesięciu połączeń w pętli każde tylko po to żeby wstawić jeden rekord musi skończyc się źle. Zresztą mamy bazę działającą pewnie na jakimś niespecjalnie wydajnym serwerze, a mówimy to o odpaleniu kilki do kilkudziesięciu połączeń w przedziale poniżej 1ms. Problem z pewnością leży w równoległym otwieraniu tych połączeń (nie jest wiadome czy każde z nich to osobny wątek) bo dodanie Result - czyli wymuszenie synchroniczności naprawiło błąd.

Dodanie Result, czyli tworzenie synchronicznego wrappera na asynchroniczny kod, to akurat przyczyna błędu. Tworzenie wielu obiektów SqlConnection nie jest dobrą praktyką, ale też nie jest jakimś dużym problemem, bo .NET sam zajmuje się connection poolingiem.

wloochacz napisał(a):

Ja to rozumiem jako zakleszczenie po stronie MSSQL.
MSSQL w przypadku zakleszczenia zwraca odpowiedni wyjątek, o taki:
Transaction (Process ID XYZ) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.

@wloochacz: Ja na bazach jakoś wybitnie się nie znam, ale zakleszczenie na samych insertach? To jest w ogóle możliwe?

Nie, to nie jest możliwe, jeśli to naprawdę tylko prosty insert into do tabeli.
Ale nie wiem, czy ów DAL (tu mamy pod spodem Dappera) nie robi w trakcie wykonywania komend dodających dokument jakiś selectów.
Poza tym nie wiem czy na owej bazie są jakieś triggery, które mogą robić kuku.
Podłączyłby chłop SQL Profilera, to by się dowiedział, a tak to...

Nie znam się na .net, ale z perspektywy MS SQLa, może chodzić o deadlocki na foreign keys (przykładowy scenariusz: https://www.sqlservergeeks.com/sql-server-foreign-key-deadlocks/)

Z tego kodu (czysta spekulacja) może chodzić o taką sytuację:

• na osobnym wątku leci insert (wstawia rekord parent z jakimś PK, ale commita transakcji jest gdzieś poza tym kodem, który autor wstawił?)
• na osobnych połączeniach lecą inserty do tabelki "child", która odwołuje się do tabelki nadrzędnej (via foreign key)

Wątek child czeka, aż parent się zakończy (ale gdzie jest zatwierdzenie transakcji dla parenta?)

Cały stacktrace wyjątku

2019-10-29 08:41:12.6166 ERROR System.Data.SqlClient.SqlException (0x80131904): Transaction (Process ID 437) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.
   at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at System.Data.SqlClient.SqlDataReader.TryHasMoreRows(Boolean& moreRows)
   at System.Data.SqlClient.SqlDataReader.TryReadInternal(Boolean setTimeout, Boolean& more)
   at System.Data.SqlClient.SqlDataReader.<>c__DisplayClass190_0.<ReadAsync>b__1(Task t)
   at System.Data.SqlClient.SqlDataReader.InvokeRetryable[T](Func`2 moreFunc, TaskCompletionSource`1 source, IDisposable objectToDispose)
--- End of stack trace from previous location where exception was thrown ---
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in C:\projects\dapper\Dapper\SqlMapper.Async.cs:line 437
   at API.Services.SQLService.QueryCompany[T](String query, Object param) in D:\PROJEKTY\VENTS GROUP\API\API\Services\ISQLService.cs:line 182
   at API.Controllers.DataController.GetDokumenty(DateTime dateFrom, DateTime dateTo, String type) in D:\PROJEKTY\VENTS GROUP\API\API\Controllers\DataController.cs:line 309
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at API.Utils.CustomExceptionMiddleware.ExceptionMiddleware.InvokeAsync(HttpContext httpContext) in D:\PROJEKTY\VENTS GROUP\API\API\Utils\CustomExceptionMiddleware\ExceptionMiddleware.cs:line 34
ClientConnectionId:fc740281-f816-4170-b15a-cc0414051d3d
Error Number:1205,State:51,Class:13

Ciekawsze niż stack trace aplikacyjny, byłoby uzyskanie informacji po stronie serwera bazodanowego. Szybkie google pokazuje, że narzędzia sqlserverowe potrafią produkować rysunki obrazujące deadlock: https://www.red-gate.com/simple-talk/sql/learn-sql-server/how-to-track-down-deadlocks-using-sql-server-2005-profiler/