Switched to a more performant locking library

[MarkCiliaVincenti]

Disclaimer: I am the author of the new library.

Public benchmarks running on GitHub show AsyncNonKeyedLocker to be considerably more performant; in tests NeoSmart.AsyncLock took 9.71x the time and allocated 7.42x the memory.

https://github.com/MarkCiliaVincenti/AsyncNonKeyedLockBenchmarks/actions/runs/7526873065/job/20485876144

[MarkCiliaVincenti]

@tonysneed

[MarkCiliaVincenti]

By the way there's a 6.4.2 version out since these PRs

[tonysneed]

Running these tests with steps 2-4 results in a deadlock. Can you investigate?

EventDriven.Sagas/test/EventDriven.Sagas.Abstractions.Tests/Saga/SagaTests.cs

Line 21 in 70f5c3b

public async Task SagaShouldCompleteToStep(int step)

[MarkCiliaVincenti]

Are you perhaps using lock re-entrancy?

…

On Mon, 22 Apr 2024, 20:20 Anthony Sneed, ***@***.***> wrote: Running these tests with steps 2-4 results in a deadlock. Can you investigate? https://github.com/event-driven-dotnet/EventDriven.Sagas/blob/70f5c3b61ea7fc11a5146d3a8e936482847b6fb7/test/EventDriven.Sagas.Abstractions.Tests/Saga/SagaTests.cs#L21 — Reply to this email directly, view it on GitHub <#14 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF7U7YBCYKBDTZ5EYDDZSGLY6VIHXAVCNFSM6AAAAABCIROP7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZQGUZDONBXHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[MarkCiliaVincenti]

Yes you are using lock reentrancy @tonysneed. Please look at the code now, but be careful as I'm seeing some CurrentStep--... is there a risk that it would enter twice with CurrentStep == 1? ConditionalLockAsync is a new feature and will only lock whilst the condition is true. Not real reentrancy but is often enough. Reentrancy and async code are at best challenging; some even claim impossible (https://itnext.io/reentrant-recursive-async-lock-is-impossible-in-c-e9593f4aa38a)

[MarkCiliaVincenti]

Is it OK @tonysneed ?

[tonysneed]

There are other tests failing due to deadlocks. Not sure how ConditionalLockAsync addresses reentrancy issues or whether the code needs to be refactored.

[MarkCiliaVincenti]

It is always a good idea to refactor code to avoid reentrancy in async code. As I wrote before it is at least challenging. ConditionalLockAsync doesn't even try that. It's a simple 'if' statement... if the condition is true, it waits on the semaphore and if the condition is false it doesn't (i.e. enters immediately). That means you'd need to do the bookkeeping yourself and telling it yourself that it's a 'reentrancy'.

[MarkCiliaVincenti]

@tonysneed I spent some time trying to understand how the code works but it's too complex and in essence I didn't understand what needs to be locked from what.

[tonysneed]

Appreciate the info on the impracticality of reentrancy with synchronization using async await.

I'll create an issue to look into the need for synchronization on Saga.TransitionSagaStateAsync. In the meantime, I'll put this PR on hold.

[MarkCiliaVincenti]

Played around with an experiment and got the tests to pass through the use of AsyncLocal, but I'd still try to avoid reentrancy as AsyncLocal could lead to problems.