FoundatioFx / Foundatio

Pluggable foundation blocks for building distributed apps.
Apache License 2.0
1.99k stars 244 forks source link

Replace with async lock #294

Closed garcipat closed 1 year ago

garcipat commented 1 year ago

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:

garcipat commented 1 year ago

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

ejsmith commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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