CODEWITHSUNDEEP
javamultithreadingconcurrency
Hallow
Hallow

Reputation: 1070

Can a thread that is running see changes in private fields?

I am trying to implement a kind of thread pool, that when I start a Thread, my own class that extends thread, its run method will run forever or until some condition is met.

  1. While the thread is running it should receive work, as Callable interface.
  2. When it retrieves the value it should notify a Condition variable.
private class Worker extends Thread {
    private Callable<T> toCall;
    private T result;

    @Override
    public void run(){
        try{
            while(true) {
                if(toCall == null && /* Other condition accessing Outer class field */){
                    // Idle for too much time
                    break; // Finish thread run normally
                }

                if(toCall != null && /* Other condition accessing Outer class field */){
                    // Is this block reached when setCallable is invoked after this thread is running ? 
                    result = toCall.call();
                    hasResult.signal(); // Condition variable
                    toCall = null;
                }
            }
        }catch(InterruptedException ie){
            // TODO 
        }
    }

    public void setCallable(Callable<T> toCall){
        this.toCall = toCall;
    }

    public void getResult(){
        return result;
    }
}

My main doubts are:

I'm trying to implement my own very simple ThreadPoolExecutor and my approach is to have a list of these Worker's, creating them upon need, and reusing those from this list that aren't doing anything.

If this approach is wrong whats the best way to achive this functionality?

Any help would be appreciated, thanks.

Upvotes: 2

Views: 159

Answers (2)

Marco13
Marco13

Reputation: 54679

As already pointed out by AR.3 in his answer, the thread may not necessarily see the change in the toCall variable.

Declaring the variable as volatile is one way of making this point safe, but (without diving too deeply into the details of the Java Memory Model), this may not be necessary. More precisely: It may not be sufficient here, because there clearly is a race condition involving your toCall variable:

   | Worker thread:                            |  External thread:
---| ------------------------------------------|--------------------------------
0: | while(true) {                             |
1: |                                           |  worker.setCallable(c);
2: |     if(toCall != null){                   |
3: |                                           |  worker.setCallable(null);
4: |         result = toCall.call(); // CRASH! |
5: |

So an external thread may set the toCall variable to a non-null value. The worker thread may find that toCall!=null, and enter the body of the if-statement. At this point, an external thread may set toCall to null, causing a NullPointerException when the worker attempts to do toCall.call().

Of course, this could be fixed by wrapping accesses to the toCall variable into synchronization blocks. And then, it would not have to be volatile any more - that's what I referred to above.

There may be some other synchronization issues with your code. You did not give us all details, particularly about the role of the hasResult variable. And you did not say whether there is a particular reason of why you wanted to implement this on your own.

But in general:

There are easier, more elegant and more idiomatic ways of achieving what you probably intended. One of the comments already pointed you to the Future class, which offers the functionality that you seemingly wanted to implement there. Depending on where and how the synchronization (i.e. the waiting, in this case) should take place, there are different options. The easiest could be an executor service:

ExecutorService executorService = Executors.newFixedThreadPool(1);
Future<T> future = executorService.submit(toCall);

// Whereever this should be consumed:
T t = future.get(); // Will wait until the result is computed

Alternatively, the CompletableFeature class offers mechanisms for adding a "callback" that will be informed when the result is avaliable.

Upvotes: 1

M A
M A

Reputation: 72884

Does this thread see the change in toCall variable ?

In terms of visibility, with no synchronization whatsoever, the answer is not necessarily.

after it uses this variable and sets it to null, can another call to setCallable be safe ?

This is where the implementation may not be "safe". What happens if two threads submit a Callable at the same time to the worker? Given the above code, it will process only one of them and ignore the other. A better alternative may be to have the workers poll a shared queue of tasks. You can use something like a BlockingQueue to save the tasks. One of the methods there allow you to poll the queue with a timeout.

Finally is this a good way of having a thread always run and give it work when needed?

I don't think so, for the reason mentioned above.

Upvotes: 2

Related Questions