0

I need to run 2 threads simultaneously (on occasion if both have been requested together) otherwise if either one is requested solo then each thread needs to run on its own. Each thread will be responsible for taking its own unique reading from its own unique transducer (actual hardware), I need each thread to query its transducer until a certain value is detected. If and only if this value is detected should the thread stop and exit.

I also need a way for my software to know without a doubt that both threads have stopped and exited/completed their task and detected their respective values from the hardware. If and only if both threads detect their values should my onFinish() method be called. In my code, I used a third thread to do this monitoring and the use of integer values. The integer variable threadCount is reset to 0 once my onFinish() method is called and so is my boolean shouldRun which is reset to false within the onFinish() method.

Was wondering if my approach is acceptable/sound logically ( please note I havent done the logic yet for the actual query of each transducer (likely will use a while loop)) also what are the consequences for using the approach I described, my code is as seen below:

private void decreaseThreadCount(){
    threadCount -=1;
}

boolean shouldRun = false;
int threadCount = 0;

public void onStart() {
    System.out.println("START");
    System.out.println("    ");
    System.out.println("START PROGRESS BAR");

    if((customProgressBarL != null || customProgressBarR != null) || (customProgressBarL != null && customProgressBarR != null)){
        shouldRun = true;
    }

    /**TESTING PURPOSES*/

    if (customProgressBarL != null) {
        threadCount += 1;
        new Thread(new Runnable() {
            @Override
            public void run() {
                for (int i = 0; i <= 100; i++) {
                    try {
                        customProgressBarL.updateProgress(i);
                        customProgressBarL.repaint();
                        Thread.sleep(50);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }

                try {
                    Thread.sleep(5);
                    decreaseThreadCount();
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }

        }).start();
    }

    if (customProgressBarR != null) {
        threadCount += 1;
        new Thread(new Runnable() {
            @Override
            public void run() {

                for (int i = 0; i <= 100; i++) {
                    try {
                        customProgressBarR.updateProgress(i);
                        customProgressBarR.repaint();
                        Thread.sleep(50);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
                //System.out.println("Thread Count: " + threadCount);
                try {
                    Thread.sleep(5);
                    decreaseThreadCount();
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }).start();
    }

    new Thread(new Runnable() {
        @Override
        public void run() {

            while(threadCount >= 0 && shouldRun){
                try {
                    System.out.println("Thread Count: " + threadCount);
                    if(threadCount == 0){
                        onFinish();
                        return;
                    }
                    Thread.sleep(50);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
            return;
        }
    }).start();

}

[Edit 1] After reading through the advice given and a bunch of online documentation I have come up with the following code which seems to work under the cases I have tested so far.

Was wondering if I should still use SwingWorkers or if the modified approach is acceptable?

public void onStart() {
    System.out.println("START");
    System.out.println("    ");
    System.out.println("START PROGRESS BAR");

    /**TESTING PURPOSES*/
    CountDownLatch countDownLatch = new CountDownLatch(2);
    new Thread(new Runnable() {
        @Override
        public void run() {
            new Thread(new Runnable() {
                @Override
                public void run() {
                    if (customProgressBarL != null) {
                        for (int i = 0; i <= 100; i++) {
                            try {
                                customProgressBarL.updateProgress(i);
                                customProgressBarL.repaint();
                                Thread.sleep(50);
                            } catch (InterruptedException e) {
                                e.printStackTrace();
                            }
                        }
                    }
                    countDownLatch.countDown();
                    return;
                }
            }).start();

            new Thread(new Runnable() {
                @Override
                public void run() {         
                    if(customProgressBarR != null){
                        for (int i = 0; i <= 100; i++) {
                            try {
                                customProgressBarR.updateProgress(i);
                                customProgressBarR.repaint();
                                Thread.sleep(50);
                            } catch (InterruptedException e) {
                                e.printStackTrace();
                            }
                        }
                    }
                    countDownLatch.countDown();
                    return;
                }
            }).start();
            try{
                countDownLatch.await();
                onFinish();
            } catch (InterruptedException e){
                e.printStackTrace();
            }
        }
    }).start();
}

[Edit 2]

I have tried a version where I use SwingWorker instead of my Threads, below is the code and it works just as well as the code in [Edit 1] (as far as I can tell at least). What are the pros/cons of each approach?

 private class MySwingWorker extends SwingWorker<Object, Object> {

    CountDownLatch countDownLatch;
    JCustomProgressBar progressBar;

    public MySwingWorker(CountDownLatch countDownLatch, JCustomProgressBar progressBar){
        this.countDownLatch = countDownLatch;
        this.progressBar = progressBar;
    }

    @Override
    protected Object doInBackground() throws Exception {
        if(progressBar != null){
            for (int i = 0; i <= 100; i++) {
                try {
                    progressBar.updateProgress(i);
                    progressBar.repaint();
                    Thread.sleep(50);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
        countDownLatch.countDown();
        return null;
    }
}

private class MySwingWorkerManager extends SwingWorker<Object, Object> {

    CountDownLatch countDownLatch;

    @Override
    protected Object doInBackground() throws Exception {
        this.countDownLatch = new CountDownLatch(2);
        new MySwingWorker(countDownLatch, customProgressBarL).execute();
        new MySwingWorker(countDownLatch, customProgressBarR).execute();
        try{
            countDownLatch.await();
            onFinish();
        } catch (InterruptedException e){
            e.printStackTrace();
        }
        return null;
    }
}

I initiate everything as follows in my onStart() method by calling the execute() method:

public void onStart() {
    System.out.println("START");
    System.out.println("    ");
    System.out.println("START PROGRESS BAR");

    System.out.println("customProgressBarL is: "+customProgressBarL);
    System.out.println("customProgressBarR is: "+customProgressBarR);

    /**TESTING PURPOSES*/
    new MySwingWorkerManager().execute();
}
Thornack
  • 51
  • 1
  • 6
  • The one thing I noticed with my approach is that on occasion my decreaseThreadCount() is not called after both threads finish which results in things hanging. Not sure how to get around this – Thornack Jan 07 '19 at 06:48
  • Well, you're violating the single threaded nature of the Swing API to start with, risking possible dirty read/writes/paints – MadProgrammer Jan 07 '19 at 06:52
  • 1
    Since `threadCount` is not `volatile`, you're also risking dirty reads of the variable. The free wheeling thread (while(threadCount >= 0 && shouldRun){`) is a bad idea and should be managed with either a monitor lock or something like a `CountDownLatch` – MadProgrammer Jan 07 '19 at 06:53
  • 1
    You might also want to have a read of [Concurrency in Swing](https://docs.oracle.com/javase/tutorial/uiswing/concurrency/index.html) – MadProgrammer Jan 07 '19 at 07:52
  • This [example](https://stackoverflow.com/a/11372932/230513) illustrates `CountDownLatch`. – trashgod Jan 07 '19 at 18:00
  • I posted what I came up with. Question is, do I have to use SwingWorkers or is what I am doing in the revised code acceptable? Seems to work under the cases I have tested so far. I implemented the CountDownLatch successfully and that seems to work ok. Do I have to do something like this as well? SwingUtilities.invokeLater(() -> { customProgressBarR.updateProgress(i); customProgressBarR.repaint(); }); – Thornack Jan 08 '19 at 23:17

2 Answers2

2

You've initiated all the threads simultanesouly outside (T1,T2,T3) and T3 is waiting for all of them to finish and do some onFinish.

Instead you can instantiate T1 and T2 inside T3. Create a countdown latch with the number of count. Decrement the latch count when both the threads finish their task and then execute onFinish().

All the logic to create T1/T2 should be inside T3.

new Thread(new Runnable() {
    @Override
    public void run() {
      int count = 2;
      CountdownLatch latch = new CountdownLatch(count);
      MyThread t1 = new MyThread(latch);
      MyThread t2 = new MyThread(latch);
      t1.start()
      t2.start();
      latch.await();
      onFinish();
    }
}).start();
munish
  • 453
  • 6
  • 22
  • if I put T1 and T2 in T3, does this solve the issues everyone mentioned above? – Thornack Jan 07 '19 at 07:08
  • 1
    The issues other guys mentioned is that the threadCount is not threadSafe. Means due to race condition it can be like, both the threads read the value as 2 tried to decrement simultaneously and now the current value is 1 instead of 0. To overcome this, I've used countdown latch which is threadsafe. Also the threads of T1/T2 should look like this. run(){ – munish Jan 07 '19 at 09:21
  • 1
    Swing applications (it is not clear we are talking about swing here but we should pay attention to the tag) should not open Threads here and there. Swing applications should use SwingWorkers instead. – George Z. Jan 07 '19 at 09:37
  • To clarify, my application is written using Swing, and from the comments above I gather that I cannot multi-thread if using Swing as it is intended for single thread only? – Thornack Jan 08 '19 at 01:03
  • I have implemented your solution and it seems to work pretty well, thanks! I posted my code above in the edit portion of my post. I was wondering if I have to still use SwingWorkers instead of creating my threads as seen above? Also, do I need to use [Code] SwingUtilities.invokeLater(() -> { customProgressBarR.updateProgress(i); customProgressBarR.repaint(); }); [/Code] as was suggested? – Thornack Jan 09 '19 at 03:36
0

Only one thread is supposed to access swing components; you should write something like that:

SwingUtilities.invokeLater(() -> {
    customProgressBarR.updateProgress(i);
    customProgressBarR.repaint();
});
Maurice Perry
  • 9,261
  • 2
  • 12
  • 24