Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switched from AsyncDuplicateLock to AsyncKeyedLock #191

Open
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

MarkCiliaVincenti
Copy link

Switched from AsyncDuplicateLock to AsyncKeyedLock, which provides better performance and lower memory usage.

@MarkCiliaVincenti
Copy link
Author

@nickevansuk

@MarkCiliaVincenti
Copy link
Author

@nickevansuk nudge :)

@MarkCiliaVincenti
Copy link
Author

@nickevansuk

@nickevansuk
Copy link
Contributor

Thanks for this @MarkCiliaVincenti, very helpful - note that it has previously failed in CI for .NET Framework. Have approved a new run so fingers crossed this time it passes... otherwise it probably just needs a dependency update in the .NET Framework project.

@MarkCiliaVincenti
Copy link
Author

Thanks for this @MarkCiliaVincenti, very helpful - note that it has previously failed in CI for .NET Framework. Have approved a new run so fingers crossed this time it passes... otherwise it probably just needs a dependency update in the .NET Framework project.

Does it usually take this long?

@MarkCiliaVincenti
Copy link
Author

OK, made some changes, try again now @nickevansuk?

@MarkCiliaVincenti
Copy link
Author

@nickevansuk framework ones are failing halfway through. I see it's been a long time since these tests have passed. I'm not convinced the changes I made have broken anything. What are your thoughts?

@MarkCiliaVincenti
Copy link
Author

@nickevansuk

@MarkCiliaVincenti
Copy link
Author

Still issues with #192, can add that to the backlog

I'm not sure I understand what you mean, could you please rephrase?

@nickevansuk
Copy link
Contributor

It looks like there's some issue with .NET Framework that requires some investigation on our side? Is that right? If so have added it to the backlog

@MarkCiliaVincenti
Copy link
Author

It looks like there's some issue with .NET Framework that requires some investigation on our side? Is that right? If so have added it to the backlog

There must be. I guess that's a prerequisite for this getting merged?

@nickevansuk
Copy link
Contributor

Afraid so - we can't release something that fails CI

@MarkCiliaVincenti
Copy link
Author

@nickevansuk any luck so far?

@nickevansuk
Copy link
Contributor

Nothing yet - hoping to get some time to look in the next couple of weeks. It's a deep issue so needs some proper investigation.

@MarkCiliaVincenti
Copy link
Author

Nothing yet - hoping to get some time to look in the next couple of weeks. It's a deep issue so needs some proper investigation.

OK, I updated to 6.2.0 meanwhile although there are no changes that affect you unless you would consider using the new striped locking technique instead.

@MarkCiliaVincenti
Copy link
Author

@nickevansuk any luck yet?

@MarkCiliaVincenti
Copy link
Author

@nickevansuk do you intend on fixing the persistent issue or shall I close the PR?

@MarkCiliaVincenti
Copy link
Author

Can you please try now @nickevansuk ?

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.

2 participants