patterncsharpMinor
How thread safe is this class?
Viewed 0 times
thisthreadsafehowclass
Problem
So the way I understand this code I wrote, it is thread safe, as long as the retrievers and depositors are thread safe. The only bad state I could see occurring is that a thread is using a retriever as a method local while another thread calls RefreshDependencies(), which would mean the thread with it as a method local would continue to use it even though it wasn't one of the current depositors/retrievers, but that one should be cleaned up when the method local using it exited as there would be no root anymore.
also open to any other notes on the code.
also open to any other notes on the code.
public static class DependencyContainer
{
private const string DEPENDENCIES_LOCATION_APP_SETTINGS_KEY = "DependenciesLocation";
private static object _lockDependenciesManagement;
private static string _dependenciesLocation;
public static IEnumerable Retrievers { get; private set; }
public static IEnumerable Depositors { get; private set; }
static DependencyContainer()
{
_lockDependenciesManagement = new object();
string dependenciesLocation = ConfigurationManager.AppSettings[DEPENDENCIES_LOCATION_APP_SETTINGS_KEY] ?? typeof(DependencyContainer).Assembly.CodeBase.Replace(@"file:///", "");
SetDependenciesLocation(dependenciesLocation);
RefreshDependencies();
}
public static string GetDependenciesLocation()
{
lock (_lockDependenciesManagement)
{
return _dependenciesLocation;
}
}
public static void SetDependenciesLocation(string newDependenciesLocation)
{
lock (_lockDependenciesManagement)
{
_dependenciesLocation = newDependenciesLocation;
}
}
public static void RefreshDependencies()
{
lock (_lockDependenciesManagement)
{
// compose/load dependencies in here for writing and retrieving blog entries
}
}
}Solution
First item I see is:
...should be:
I don't see any reason this should/would change.
With the possibility of having
Your consumers can now manipulate work from it's own copy of the _retrievers collection. Handle
private static object _lockDependenciesManagement;...should be:
private static readonly object _lockDependenciesManagement;I don't see any reason this should/would change.
With the possibility of having
Retrievers overwritten while being used, you should return a copy of the collection instead:// fields that could be altered while in use if made public
static IEnumerable _retrievers { get; private set; }
static IEnumerable _depositors { get; private set; }
public static IEnumerable Retrievers
{
get
{
// return copy of _retrievers
lock (_lockDependenciesManagement)
{ return _retrievers.ToList(); }
}
}Your consumers can now manipulate work from it's own copy of the _retrievers collection. Handle
_depositors in the same manner ...I think these steps will make the collections safer to use.Code Snippets
private static object _lockDependenciesManagement;private static readonly object _lockDependenciesManagement;// fields that could be altered while in use if made public
static IEnumerable<IEntryRetriever> _retrievers { get; private set; }
static IEnumerable<IEntryDepositor> _depositors { get; private set; }
public static IEnumerable<IEntryRetriever> Retrievers
{
get
{
// return copy of _retrievers
lock (_lockDependenciesManagement)
{ return _retrievers.ToList(); }
}
}Context
StackExchange Code Review Q#4435, answer score: 4
Revisions (0)
No revisions yet.