1
\$\begingroup\$

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();
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ CodeReview forum is probably not the best for this type of questions, you'll get more attention on StackOverflow \$\endgroup\$
    – almaz
    Commented Jan 25, 2013 at 22:20
  • 1
    \$\begingroup\$ I realize it's just an example, but I think factoryClass is a very bad name for a variable. \$\endgroup\$
    – svick
    Commented Jan 25, 2013 at 22:27

3 Answers 3

2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jan 26, 2013 at 4:19
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – svick
    Commented Jan 26, 2013 at 12:30
  • 1
    \$\begingroup\$ 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 \$\endgroup\$
    – almaz
    Commented Jan 26, 2013 at 17:31
  • \$\begingroup\$ Hmm, you're right. Somehow, I got confused. \$\endgroup\$
    – svick
    Commented Jan 26, 2013 at 17:37

Your Answer

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

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