5
\$\begingroup\$

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.

public static class DependencyContainer
{
 private const string DEPENDENCIES_LOCATION_APP_SETTINGS_KEY = "DependenciesLocation";
 private static object _lockDependenciesManagement;
 private static string _dependenciesLocation;
 public static IEnumerable<IEntryRetriever> Retrievers { get; private set; }
 public static IEnumerable<IEntryDepositor> 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
 }
 }
}
asked Aug 27, 2011 at 17:20
\$\endgroup\$
2
  • 1
    \$\begingroup\$ 'it is thread safe, as long as the retrievers and depositors are thread safe.' ...the class is either thread-safe or it isn't, IMO ;) \$\endgroup\$ Commented Aug 27, 2011 at 17:28
  • \$\begingroup\$ I mean, the class is thread safe- though that does not guarantee the retrievers or depositors are. The implementor of those has to make them independently thread safe. \$\endgroup\$ Commented Aug 27, 2011 at 22:16

1 Answer 1

4
\$\begingroup\$

First item I see is:

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<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(); }
 }
}

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.

answered Aug 27, 2011 at 17:31
\$\endgroup\$
5
  • 1
    \$\begingroup\$ You should lock _retrievers.ToList(), too or you defeat the point of using the locks. \$\endgroup\$ Commented Aug 27, 2011 at 20:37
  • \$\begingroup\$ Good point on making the lock object readonly, though I'm not sure there's benefit in the .ToList() as the IEnumerable's cannot be manipulated by consumers, IEnumerable just implements read operations, manipulations would be done to the retriever elements which would be the same ones referenced in the .ToList \$\endgroup\$ Commented Aug 27, 2011 at 22:15
  • \$\begingroup\$ Correct me if i'm wrong, but every thread calling getenumerator on an enumerable, gets its own new enumerator right? \$\endgroup\$ Commented Aug 27, 2011 at 22:54
  • \$\begingroup\$ @Jimmy: 'every thread calling getenumerator on an enumerable, gets its own new enumerator right?' ...yes, that is the intent. \$\endgroup\$ Commented Aug 28, 2011 at 3:13
  • \$\begingroup\$ After thinking about it, you're right, I think it's thread safe without this immutable style, but I know it's thread safe the way you did it. Smartass. Will put the lock around it too. Glad I asked on here. You can see my changes at github.com/JimmyHoffa/IndyBlog \$\endgroup\$ Commented Aug 28, 2011 at 13:08

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.