1

There is a thread that keeps tracking new emails while a static variable tracking is true. It looks like this:

Thread thread = new Thread(new Runnable() {

    @Override
    public void run() {
        try  {
            tracking = true;
            while(tracking){
                //check emails
             }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
});

I implemented a function to stop tracking emails, that basically sets tracking to false.

Tracking is just a private static boolean which I use only inside the class that does these tasks.

Can this approach lead me to any problem?

Daniel
  • 7,357
  • 7
  • 32
  • 84

3 Answers3

4

Can this approach lead me to any problem?

Yes it can.

What you are facing is an issue of "visibility". At the highest level, "visibility" is the ability for your thread to see modifications that are performed by other threads. If you want to achieve visibility, then, it terms of the Java specification, you need to establish a "happens before relationship" between the value reads, and the value writes. Without such a relationship, there is no guarantee that any write to the tracking variable can be seen by any other thread than the one that performed the write.

In your case, two possibilities come to mind to establish the happens before relationship :

  1. You make your boolean volatile at its declaration (no further change to your program is required)
  2. You transform your boolean into an AtomicBoolean (you may then declare it final, and either use tracking.set(false) to disable tracking and tracking.get() to check if you should still track).

To be clear : no need to perform both changes, it's one or the other.

Other possibilities include : protecting your read and writes with a java.util.Lock but this is overkill.

You can read more here : How to understand happens-before consistent

GPI
  • 9,088
  • 2
  • 31
  • 38
3

If there are several threads that read and write to the same variable you need to use synchronization (e.g. an AtomicBoolean). In this particular case it is enough to declare tracking as volatile so that new value set to that variable in one thread will be correctly visible in another thread:

private static volatile boolean tracking;
Michael
  • 41,989
  • 11
  • 82
  • 128
Ivan
  • 8,508
  • 2
  • 19
  • 30
3

If this is the only thread using tracking and you are just using it as a way to terminate this one thread then you should probably be fine, though it's still iffy. When you set tracking to false it might loop a few more times but it will eventually read false (unless it gets set back to true somewhere as explained below) and it will terminate.

But if there are multiple threads that use tracking then this could definitely be a problem for you, even if you used volatile, or AtomicBoolean.

If you set tracking to false, but immediately after that another of your Runnables starts running and sets tracking back to true,

 public void run() {
    try  {
        tracking = true; // <-- sets tracking back to true
        while(tracking){
            //check emails
        }

then the threads that you were attempting to end would not end since tracking is true again.

You would need to make sure that all threads have properly stopped, before setting tracking back to true again. Synchronization and locks can help with this.

xtratic
  • 4,600
  • 2
  • 14
  • 32
  • 1
    Agreed that without further guarantees about when / how threads are launched and the tracking variable reset, you may encounter this issue "on top of" / "regardless of" visibility issues. – GPI Oct 29 '18 at 14:34