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

Replace with async lock #294

Merged
merged 4 commits into from
Sep 24, 2023
Merged

Conversation

garcipat
Copy link
Contributor

@garcipat garcipat commented Sep 23, 2023

In FolderFileStorage and InMemoryFileStorage, the lock wasn't async. Replaced it with the available AsyncLock recommended in this issue: #293

There are two open points:

  • Should the FolderFileStorage also use the async lock in all methods since its only used in Copy and Rename
  • if the InMemoryFileStorage should use a ConcurrentDictionary instead that is threadsafe by nature.

Copy link
Contributor

@ejsmith ejsmith left a comment

Choose a reason for hiding this comment

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

I like these changes. Nice work! Thanks @garcipat

@ejsmith ejsmith merged commit 0cfb6f0 into FoundatioFx:master Sep 24, 2023
2 checks passed
@garcipat
Copy link
Contributor Author

@ejsmith what do you think about the two points I mentioned in the description?

@ejsmith
Copy link
Contributor

ejsmith commented Sep 24, 2023

@garcipat sorry, missed those questions. I'm not sure about using locking in more places. Is it really needed? I can see using a lock on a rename operation, but I'm not convinced we need to be locking in other places. What are your thoughts?

@garcipat
Copy link
Contributor Author

@garcipat sorry, missed those questions. I'm not sure about using locking in more places. Is it really needed? I can see using a lock on a rename operation, but I'm not convinced we need to be locking in other places. What are your thoughts?

@ejsmith I thought that it would be required as soon as you access files since you are maybe accessing the same file with two operations. I think the azure blob storage is doing that internally, but in the InMemory the lock is present in nearly every operation. So I thought its maybe necessary also in the FolderFileStorage. But was just a thing I saw while replacing the sync lock. Its probably not critical, otherwise it would have been reported earlier.

What do you think about replacing the lock with a ConcurrentDictionary instead of having the lock everywhere?

@ejsmith
Copy link
Contributor

ejsmith commented Sep 25, 2023

@garcipat I think it would be fine to do so. I'm not sure it's necessary, but I'd be good with that change if you want to do it.

@garcipat
Copy link
Contributor Author

I will see if I find time for it. It's definetly not urgent.

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.

None yet

2 participants