Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

If I need to create a IDisposeable object from a factory, but if the factory object is not thread safe and requires me to lock on it is this the correct pattern to use?

public void DisposeExample(FactoryClass factoryClass)
{
    DispObject dispObject = null;

    lock(factoryClass)
    {
        dispObject = factoryClass.GetDispObject();
    }

    using(dispObject)
    {
        dispObject.DoWork();
    }
}
share|improve this question
1  
CodeReview forum is probably not the best for this type of questions, you'll get more attention on StackOverflow –  almaz Jan 25 '13 at 22:20
1  
I realize it's just an example, but I think factoryClass is a very bad name for a variable. –  svick Jan 25 '13 at 22:27

3 Answers 3

up vote 2 down vote accepted

Yes, assuming the factory is shared between threads and the created object doesn't contain any resource that is shared with other objects from the same factory.

But as was already pointed out in your previous question, a better solution might be to have a separate factory for each thread. Factories usually don't use much resources, so it should be fine to have more of them.

share|improve this answer
    
Thank you, no one actually answered my question in my previous question so I had to re-ask it without the distraction of the AES implementation as I already said I would be doing it the recommended way (and have already changed my code to do so), but I still wanted to know how to handle the situation if it ever came up again. –  Scott Chamberlain Jan 26 '13 at 4:19

If your question is "will the dispObject get disposed?", the answer is "Yes, that's a valid use of using." See the using reference, third sample under remarks.

share|improve this answer

It's a common mistake (not present in the code you provided but still possible, warning just in case) that people erroneously think that if you put a lock in some place all other places where object is about to be used will automagically wait until the lock is released. You need ensure you enter the lock in all places that use your factory, that is your object should own the factory (see lock for more details).

As to your code - technically speaking it is correct because using statement won't throw exceptions if disposable object happens to be null. But since the method is public and factory is passed from outside you break the rule of locking only on objects you own. And also there is no guarantee that other code with access to factory properly locks on it.

And finally, factories are usually objects that are thread safe (can't think of any that is not), so if you own the code for the factory I would recommend to refactor it to make it thread-safe, otherwise create a thread-safe wrapper around original factory. Or, if your factory is lightweight just create a separate factory per thread as svick suggested.

share|improve this answer
    
Factories are usually used to create objects. In that case, the method in question does own the returned object and there is no “other ocde” that can access the same object. –  svick Jan 26 '13 at 12:30
1  
The factory is the object being locked, and the method does not own it since factory is passed as a parameter to public method. The rule (described in the lock article referenced above) is that you should lock only on objects you own –  almaz Jan 26 '13 at 17:31
    
Hmm, you're right. Somehow, I got confused. –  svick Jan 26 '13 at 17:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.