Skip to content

Fixes key locking issues#155

Open
MarkCiliaVincenti wants to merge 6 commits into
btcpayserver:masterfrom
MarkCiliaVincenti:AsyncKeyedLock
Open

Fixes key locking issues#155
MarkCiliaVincenti wants to merge 6 commits into
btcpayserver:masterfrom
MarkCiliaVincenti:AsyncKeyedLock

Conversation

@MarkCiliaVincenti
Copy link
Copy Markdown

Fixes race condition mentioned in #148 and also a potential issue when token is cancelled not releasing semaphore during await Task.Delay(2500, _cts.Token); possibly causing issues.

@MarkCiliaVincenti
Copy link
Copy Markdown
Author

Fixed, @Kukks @NicolasDorier!

private static readonly AsyncDuplicateLock _locker = new();
private static readonly AsyncKeyedLocker<string> _locker = new(o =>
{
o.PoolSize = 20;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these settings? Does this restrict to only 20 concurrent locks?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the pooling is there to improve performance and reduce memory allocations by reusing objects. In case the pool is empty, an object is created. The pool is thus not a restriction.

@NicolasDorier
Copy link
Copy Markdown
Member

@MarkCiliaVincenti hey, I prefer we don't get a third party dependency for this, can you just copy the relevant classes.

@MarkCiliaVincenti
Copy link
Copy Markdown
Author

I would need to copy most classes in here, and then what, update this project whenever I update my library? I don't understand what benefit you believe you will get from using 3rd party classes as opposed to a 3rd party dll. If the reason is security, you can step into and debug the code in the NuGet package as I enabled that functionality, which allows you to confirm that what you see in the source code is the same as what is in the DLL.

@MarkCiliaVincenti
Copy link
Copy Markdown
Author

Do you want this PR closed?

@MarkCiliaVincenti
Copy link
Copy Markdown
Author

@NicolasDorier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants