Understanding threads better?

[Amandeep Singh]

When i bounce my application, there is a class named EntryManager, gets loaded only one time. This class gets loaded by 1 thread only.

Now from the ui of application, whenever i call methods in that class. All work perfectly. All requests from different users go in to that 1 time loaded
class only. So i believe calls to the methods are made from 1000 users. the call is made to the method- calculateDuties

Example-

package wow;

public class EntryManager {
public EntryManager(){
	System.out.println("Loading only 1 time");
}
public boolean calculateDuties(String ii, String jj){
	if(ii.equals("abc") && jj.equals("def")){
		return true;
	}
	return false;	
}
}

As per my requirements, now i need to use the instance variable which is defined at the class level. it is boolean NonWatch.

Question- Is this right approach to make class level variable which is not static, Which will shared by 1000 users. But inside the method,
I am initializing and reinitializing the variable for every method call. Is it possible, the boolean variable value can be interchanged between
different method calls.
I need to watch for precaution here.

package wow;

public class EntryManager {
	private boolean NonWatch = false;
public EntryManager(){
	System.out.println("Loading only 1 time");
}
public boolean calculateDuties(String ii, String jj){
	if(jj.equals("def")){
		NonWatch = true;
	}
	else{
		//reinitializing
		NonWatch = false;
	}
	if(ii.equals("abc") && jj.equals("def") && NonWatch){
		return true;
	}
	return false;	
}
}

[Amandeep Singh]

More Updates- I have seen the active count of threads inside this method it is 48. So any thread can access it.

[Steve Luke]

What is the goal of the NonWatch variable? Where is it going to be used? Why does it need to be an instance variable? What should happen if:

Thread 1 calls:
calculateDuties("lmn", "ckf");  //NonWatch should be false

Thread 2 calls:
calculateDuties("abc", "def"); //NonWatch should be true

Thread 1 wants to know what the value of NonWatch is

Given this information what should the value of NonWatch be at line 07. when Thread 1 wants to know the value of NonWatch. Is it false because the last time Thread 1 ran the code it was false? Or is it true because the last time the code was run (by anyone) the result was true?

[Amandeep Singh]

NonWatch variable is being used here.

if(ii.equals("abc") && jj.equals("def") && NonWatch){  
         return true;  
    }  

Ok i do see in your explanation, you have exposed the bug hidden.
Let's correct the code, Can we say it as now safe code ?

package wow;
public class EntryManager {
	private boolean NonWatch;
	public EntryManager(){
		System.out.println("Loading only 1 time");
	}
	public boolean calculateDuties(String ii, String jj){
		//reinitializing
		NonWatch = false;
		if(jj.equals("def")){
			NonWatch = true;
		}
		//NonWatch variable used here
		if(ii.equals("abc") && jj.equals("def") && NonWatch){
			return true;
		}
		return false;	
	}
}

[Steve Luke]

No, it is still not safe. The problem with threads is that you have no control of the order in which they occur, unless you force it to.

Let's make an even simpler example of your code:

    private boolean NonWatch;

    public boolean calculateDuties(String ii, String jj);
    {
        NonWatch = false;
        NonWatch = true;
        return NonWatch;
    }

In this case, I would expect the method to ALWAYS return true, right? Because the last thing that the method does before returning is to set NonWatch to true. But if this is run in multiple threads, then we can have a problem if Thread1 executes lines 05. and 06., then Thread2 executes line 05., then Thread1 executes line 07. The order of events would be:

1) Thread1 sets NonWatch = false
2) Thread1 sets NonWatch = true
3) Thread2 sets NonWatch = false
4) Thread1 returns false

Do you see how Thread2 pre-empted the expected results of Thread1. We expect Thread1 to return true, but it may or may not actually do so, and what it returns is actually impossible to predict because you don't know what Thread2, Thread3, or Thread4 are doing, all or any of which may change the value of NonWatch at the wrong time.

So to put it more generally, whenever you have more than 1 step in a method which relies on the correct value of a variable (which is to say the variable's use is not atomic), then the code is NOT Thread Safe unless it is local to the method or the multiple steps are synchronized to make them function as a single whole. So to fix my above simple example I could do:

    private final Object syncLock = new Object;
    public boolean calculateDuties(String ii, String jj);
    {
        synchronized(syncLock)
        {
            NonWatch = false;
            NonWatch = true;
            return NonWatch;
        }
    }

Your code has the same problem. You 'initialize' the value to true or false then later you make a comparison that relies on the value being set correctly. You can't do this, because someplace between setting it to true and making the comparison another thread may set it to false, making the whole method break (probably in both Threads).

So you could fix it a number of ways, but to know which is appropriate I would need to know why you need NonWatch at all and who is supposed to see what values. Is the latest value important? Is it the 'current thread's' value? If this is part of the web app we are talking about, would it be each user which needs to have a consistent value?

[Amandeep Singh]

So you could fix it a number of ways, but to know which is appropriate I would need to know why you need NonWatch at all and who is supposed to see what values.

NonWatch helps me to decide whether the particular piece of code needs to be executed or not. Basically it will be used in a if logic condition.
For each user, this method is called. So i believe it should not intermix with other users. So for each user, the calculation will be restricted to their passed parameters.

Is the latest value important? 

Latest value is important in the context of each user. That is the whole point i am worried of.

Is it the 'current thread's' value? If this is part of the web app we are talking about, would it be each user which needs to have a consistent value?

It is not a web application. the front end is html/javascript and the backend is pure java. Yes, each user needs to have its own consistent value.

[Steve Luke]

Amandeep Singh wrote:

Is it the 'current thread's' value? If this is part of the web app we are talking about, would it be each user which needs to have a consistent value?

It is not a web application. the front end is html/javascript and the backend is pure java. Yes, each user needs to have its own consistent value.

It doesn't matter really if the front end is HTML/javascript or not - really if you are using JSP/Strunts/JSF, etc... that amounts to HTML and javascript to the end user anyway. So if users access this program from a web page (or series of web pages) and the application itself runs on a server (JEE server) then I would consider this a web application.

Anyway, back to the question:

You have a multi-threaded application. The value must be stored for each USER, and a USER may span multiple different threads (different requests).
1) The object where NonWatch is stored should probably be stored in a scope only one USER gets to see - typically the session.
2) You still need to synchronize properly to make sure the same user accessing the method from multiple requests at the same time gets consistent results.

public class EntryManager {
	private volatile boolean NonWatch; //volatile helps publishing the value between multiple threads...
        private final Object syncLock = new Object(); //so all of this user's threads synch on the same object
	public EntryManager(){
		System.out.println("Loading only 1 time for each user");
	}
	public boolean calculateDuties(String ii, String jj){
                /* creates a local copy of the NonWatch variable so the local method doesn't have to worry about 
                 * another thread interfering.  Only works with primitives and immutable objects. 
                 */
                boolean localNonWatch = NonWatch; 

                /* The same results as your first if/else */
                localNonWatch = "def".equals(jj);

                /* The same results as your second if/else.  No need to compare using jj again, it is repetitive since you already did it. */
                boolean returnValue = "abc".equals(ii) && localNonWatch;

                /* Now that the dependent comparison is made, publish the new value of NonWatch */
                this.NonWatch = localNonWatch;

		/* Then return the results of the dependent comparison */
                return returnValue;	
	}
}

But this assumes you can have an EntryManager for each User. If that is not the case, and you can only have one EntryManager for the entire application then you are better off NOT using a variable to store the boolean value in. Instead, re-calculate it each time. It will be safer and cleaner.

[Amandeep Singh]

This code will not help me in my situation. I think in last example, you got my intention wrong.

If that is not the case, and you can only have one EntryManager for the entire application then you are better off NOT using a variable to store the boolean value in. Instead, re-calculate it each time. It will be safer and cleaner.

Yes in my situation, i am using a 1 EntryManager for the the entire application.

So i understand from you, that class variables are not safe to keep the result stored in a multi-threaded access.

The reason i am using class variable because in this way, i do not have to pass parameters in and around the methods. Just using class variable will solve my problem. Since it is a multithreaded access, things can go wrong.

my Solution-
it would be to pass local parameters only in and around the method.

out of curiosity Question- In case of beans, we do make class variables. But we do new Bean(), that's why it is safe there in case of multi-threaded applications.

[Steve Luke]

Amandeep Singh wrote:This code will not help me in my situation. I think in last example, you got my intention wrong.

If that is not the case, and you can only have one EntryManager for the entire application then you are better off NOT using a variable to store the boolean value in. Instead, re-calculate it each time. It will be safer and cleaner.

Yes in my situation, i am using a 1 EntryManager for the the entire application.

So i understand from you, that class variables are not safe to keep the result stored in a multi-threaded access.

Correct.

The reason i am using class variable because in this way, i do not have to pass parameters in and around the methods. Just using class variable will solve my problem. Since it is a multithreaded access, things can go wrong.

my Solution-
it would be to pass local parameters only in and around the method.

Good solution. And if you are having trouble with a lot of parameters being passed around, you can make a method-local object designed specifically to hold those values so you only have to pass one object around that holds the values which are needed by the different methods.

out of curiosity Question- In case of beans, we do make class variables. But we do new Bean(), that's why it is safe there in case of multi-threaded applications.

Yes. It is all about Scope. A Bean which holds instance variables would be created only in the scope were all that data should be available - once per request or once per user for example.

[Amandeep Singh]

you are so good in knowledge, that i would suggest to become Bartender soon.