search

rodion-m / ChatGPT_API_dotnet

OpenAI, Azure OpenAI and OpenRouter Chat Completions (ChatGPT) .NET integration with DI, persistence and streaming support
MIT License
77 stars 14 forks source link

Semaphore-based sync lock in EconomicalChatGPTTranslatorService #8

Closed MarkCiliaVincenti closed 5 months ago

rodion-m commented 5 months ago

@MarkCiliaVincenti Thanks a lot for the contributation! I couldn't catch, what is the benefit of AsyncNonKeyedLocker under built-in SemaphoreSlim?

I'm just trying to understand, is it really costs to add an extra dependency.

MarkCiliaVincenti commented 5 months ago

I believe that if you want the method to be truly asynchronous then you should not be using the synchronous lock.

rodion-m commented 5 months ago

I believe that if you want the method to be truly asynchronous then you should not be using the synchronous lock.

It's not always true. In these cases, lock is used for a few short operations that run on the CPU, so we don't get some valuable benefits (not to mention that SemaphoreSlim also uses a short lock under the hood when taking a lock). Also, there is a re-entrance, so with SemaphoreSlim it highly likely will be broken.

MarkCiliaVincenti commented 5 months ago

It indeed would, re-entrance and asynchronous code are always a bit of a nightmare.