Multithreading lock object, Two static public methods can call a static private method

I just thought of this.
I sense that the class above wants to become refactored into a factory pattern that returns Query objects.
Does anyone else get that, or am I misunderstanding the factory pattern?
Apologies. This was intended on being a threading question, not a design pattern question; but I just noticed that.

Thanks,
re: "If you lock a  public method , that method will act as synchronous for the time of the lock and any other methods that it calls will be also synchronous."

Yes, I would like those methods to be called synchronously because I'm worried that one of the methods might get called by the client a second time before the first one has finished. I fear then that the second client call might get the Query object that was created for the first client call.

Here is some pseudocode.

public static XType PublicMethod(XArg arg)
{
   XType publicResource;
   // Some stuff happens here.
   publicResource = ManipulateResource(arg);
   // Do stuff to publicResource. 
   return publicResource;   
}

private static XType PrivateMethod(XArg arg)
{
   lock(SpinLock)
   {
       XType privateResource = new XType();
       // Do stuff to privateResource with arg      
      return privateResource;
   }
}

Select allOpen in new window

Imagine that client1 first calls PublicMethod(), and next client2 then calls PublicMethod().
Suppose then that inside of PublicMethod(), client2 gets to call the ManipulateResources method prior to client1.
Then, while the client2 call is in the do stuff part of the code, and before privateResource gets returned for client2, the client1 call accesses the ManipulateResources method and gets a response which alters privateResource.
Then when the client2 call returns privateResource, it returns the object intended for the client1 call.
I don't know if that is something that could realistically happen, but that's what I'm worried about.

...but you haven't actually locked anything in your code.  Your "SpinLock " variable is declared locally, which means each thread will get its own copy.  Therefore each thread will run without waiting for anybody else, since nobody else has access to that local variable.

"lock" only works if multiple threads try to access the same object.  To make it work you'd have to move "SpinLock" out to class level.

Each thread will have it's own copies of all the local variables so the problem you described doesn't exist.

The only time this is an issue is if the things being manipulated are static in nature themselves.  So if the methods are changing the same static members, then yes, it's possible to get an uncertain state.  If the data being manipulated isn't getting shared somehow between threads or calls, then you don't need to worry about this.

re: "... this is an issue is if the things being manipulated are static...."

So, this pseudocode below could be a problem:

private static XType publicResource;

public static XType PublicMethod(XArg arg)
{  
   // Some stuff happens here.
   publicResource = ManipulateResource(arg);
   // Do stuff to publicResource. 
   return publicResource;   
}

private static XType PrivateMethod(XArg arg)
{
   lock(SpinLock)
   {
       XType privateResource = new XType();
       // Do stuff to privateResource with arg  
      return privateResource;
   }
}

Select allOpen in new window

One is method is manipulating a static member "publicResource".

The other is manipulating a local variable called "privateResource".  The lock in PrivateMethod isn't doing anything unless XType itself has static members inside it that need thread synchronization.

Without further knowledge, the two methods appear to be isolated with respect to each other, and there is no problem.

Now, if multiple threads were calling the first method, PublicMethod(), then you could potentially have an issue as each thread could change the state of "publicResource" without knowledge of what the others are doing to it, putting it into an invalid state.

So PublicMethod() is really the one that should have lock in it, to control access to the static member "publicResource".

Hope that explanation helps.

Ah, I am so sorry. I messed up the pseudocode.
Here are the two code pieces again below.
I changed the name of PrivateMethod to ManipulateResource so that it gets called from PublicMethod.

What I think is thread safe code:

public static XType PublicMethod(XArg arg)
{
   XType publicResource;
   // Some stuff happens here.
   publicResource = ManipulateResource(arg);
   // Do stuff to publicResource. 
   return publicResource;   
}

private static XType ManipulateResource(XArg arg)
{
   lock(SpinLock)
   {
       XType privateResource = new XType();
       // Do stuff to privateResource with arg  
      return privateResource;
   }
}

Select allOpen in new window

And what I think is not thread-safe code:

private static XType publicResource;

public static XType PublicMethod(XArg arg)
{  
   // Some stuff happens here.
   publicResource = ManipulateResource(arg);
   // Do stuff to publicResource. 
   return publicResource;   
}

private static XType ManipulateResource(XArg arg)
{
   lock(SpinLock)
   {
       XType privateResource = new XType();
       // Do stuff to privateResource with arg  
      return privateResource;
   }
}

Select allOpen in new window

I had not put the pseudocode through a compiler.
I think I got it right this time to illustrate the point behind my question.

So, the first code is thread-safe and the second is not, correct?

Would putting a lock in the second method make it thread safe?

Yes...but you'd have to make sure that no other methods are accessing "publicResource" directly, otherwise the lock becomes kinda pointless.