Go Premium for a chance to win a PS4. Enter to Win

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 609
  • Last Modified:

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

Both of the static public methods below can call the static private method.
I wasn't sure if I should put the lock on all the static methods, or just the private static method.
I can see a case why I should lock the resources in the public static methods also.
For example, what if the query object in one of the public static methods gets used by a second client call before the first client call is finished? Might that not corrupt the return argument to the second client?

Here is the code:

public static class QueryForFinder
{ 
    public static Query GetParameterQuery(
        string querySearch,
        bool returnGeometry = true)   
    {
        Query query;
        query = SetParameterQuery(querySearch, returnGeometry);
        query.OutFields.Add("*");

        return query;             
    }

    public static Query GetCollectionParameterQuery(
        string[] queryCollection, 
        string querySearch, 
        bool returnGeometry = true)
    {
        Query query;
        query = SetParameterQuery(querySearch, returnGeometry);
        query.OutFields.AddRange(queryCollection);

        return query;             
    }

    private static Query SetParameterQuery(
        string querySearch, 
        bool returnGeometry)
    {
        object SpinLock = new object();

        lock (SpinLock)
        {
            Query query = new Query();
            query.Where = querySearch;
            query.ReturnGeometry = returnGeometry;

            return query;
        }
    }
}
}

Open in new window


Should the public methods above also lock their Query object resources?
Are there any other ways to improve the code above?
0
XTO
Asked:
XTO
  • 5
  • 5
2 Solutions
 
XTOAuthor Commented:
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.
0
 
unknown_routineCommented:
No they don't have to. If your public methods were spanning new threads you may needed more locks but this is not the case for you.|

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.

Your code looks good to me . Do you see any actual issue/bug?
0
 
XTOAuthor Commented:
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;
   }
}

Open 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.
0
Free Backup Tool for VMware and Hyper-V

Restore full virtual machine or individual guest files from 19 common file systems directly from the backup file. Schedule VM backups with PowerShell scripts. Set desired time, lean back and let the script to notify you via email upon completion.  

 
Mike TomlinsonMiddle School Assistant TeacherCommented:
...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.
0
 
Mike TomlinsonMiddle School Assistant TeacherCommented:
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.
0
 
XTOAuthor Commented:
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;
   }
}

Open in new window

0
 
Mike TomlinsonMiddle School Assistant TeacherCommented:
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.
0
 
XTOAuthor Commented:
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;
   }
}

Open 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;
   }
}

Open 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?
0
 
Mike TomlinsonMiddle School Assistant TeacherCommented:
The first is thread safe, but the lock isn't necessary.  It's safe because all the variables are locally declared and each thread will create their own instances to play with.

The second is not thread safe as multiple threads could be running the "// Do stuff to publicResource" part simultaneously and making changes to the static publicResources member that don't make sense.
0
 
XTOAuthor Commented:
Would putting a lock in the second method make it thread safe?
0
 
Mike TomlinsonMiddle School Assistant TeacherCommented:
Yes...but you'd have to make sure that no other methods are accessing "publicResource" directly, otherwise the lock becomes kinda pointless.
0

Featured Post

Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

  • 5
  • 5
Tackle projects and never again get stuck behind a technical roadblock.
Join Now