-1

I have written a Thread that is dedicated to take a screenshot and convert it to jpg. So my other thread can send this screenshot parallel to another computer while the next screenshot is being taken. So far so good. It did not work to take new screenshots until I added a System.out.println... screenshot_ready... That is really weird because a println should not affect the thread.

Can you maybe explain why it only works with this system.out.println?

public void run(){
    while(continueLoop){
        System.out.println("screenshot_ready: " + screenshot_ready + "\n");
        if(!screenshot_ready){
            try {
                temp = getImageAsJPEG(robot.createScreenCapture(rectangle));
                screenshot_ready = true;
                System.out.println("screenshot created");
            } catch (ImageFormatException e) {
                e.printStackTrace();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
}

public byte[] returnJPG() {
    screenshot_ready = false;
    return temp;
}
Felix
  • 21
  • 1
  • 6
  • 2
    I don't see any synchronization. It is highly probable that the `println` is shifting timings and/or memory order so that it just happens to work. – Patricia Shanahan Aug 03 '14 at 11:58
  • What do you mean about synchronization? The continueloop is just running through and if a screenshot has been returned it has to take a new one. That is triggered by the screenshot_ready boolean. – Felix Aug 03 '14 at 11:59
  • @Felix I don't see a need for synchronization either, but I am not confident. The simplest thing would be to add synchronization delete the println and see if that works (which would strongly suggest synchronization is needed). – emory Aug 03 '14 at 12:03
  • If I understand your code correctly, your intent is to take screenshots as fast as they can be sent to another computer. – emory Aug 03 '14 at 12:06
  • But this Thread is only "one" time running and doesnt have to be synchronized as far as I see. In fact it should run the whole time through but it does only if the println is not commented out. – Felix Aug 03 '14 at 12:06
  • @emory my intention was to take screenshots parallel in another thread while another thread is sending pictures. The reason is if I do it in one thread the time for taking and converting a screenshot approx. 150ms and the time for sending them approx. 150ms is adding. I wanted to do it in different threads to get a total time of approx. 150ms because both is executed parallel. So in fact the return_JPG function call should trigger a new picture being taken because screenshot_ready is set on false. – Felix Aug 03 '14 at 12:09
  • 1
    Is `screenhot_ready` volatile? I'll bet this is a synchronization problem, but you're not showing the code for the other thread. – Kayaman Aug 03 '14 at 12:10
  • but "if(!screenshot_ready){" is only executed if I do the println. Can anyone explain it? It should run in circle with and without println – Felix Aug 03 '14 at 12:10
  • 1
    You need to consider memory order as well as synchronization, although many code constructs handle both. You have to assume that a thread may have its own, possibly out of date, copy of any data it uses, unless you have done something to ensure it has current data. – Patricia Shanahan Aug 03 '14 at 12:12
  • In the other thread I initialize the Screenshot class: screenshot = new Screenshot(robot, rect); and call it each time I want to send a screenshot to another pc: Object_Output_Stream.writeObject(screenshot.returnJPG()); – Felix Aug 03 '14 at 12:12
  • @PatriciaShanahan How can I ensure that the data is updated every time screenshot_ready is set on false again? – Felix Aug 03 '14 at 12:15
  • 1
    @Felix use http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/atomic/AtomicBoolean.html – emory Aug 03 '14 at 12:17
  • It also works if I just have these lines: System.out.print(""); if(!screenshot_ready){ That seems like a bug to me :/ – Felix Aug 03 '14 at 12:20
  • Why do you have two threads? It seems wrong. The process should take 300 ms whether you use 1 or 2 threads. Thread 1 should wait for thread 2 to finish sending the file before it starts to overwrite. – emory Aug 03 '14 at 14:01
  • 1
    @Felix There is almost certainly a bug, and it is in your code. You have multiple threads that are sharing variables, but you have no synchronization. The behavior changes when you add print statements, because this affects the timing of thread switching. I recommend you add proper synchronization to the code. – Stuart Marks Aug 03 '14 at 14:06
  • @Felix The code is not doing what it is intended to do, so there must be a bug, almost certainly in your code, specifically in a failure to correctly handle inter-thread shared data. The print statements are certainly changing timing. They could also be creating [happens-before](http://docs.oracle.com/javase/specs/jls/se5.0/html/memory.html#17.4.5) relationships between memory accesses in the threads. – Patricia Shanahan Aug 03 '14 at 14:42

1 Answers1

1

As said in the comments, using multiple threads without the correct thread safe constructs can lead to unexpected behavior, including the possibility to run without visible errors in certain circumstances.

In your case, it’s the internal synchronization inside the print stream of System.out which has a side effect on the writing thread (the thread creating the screenshot) which makes the program appear doing what you intended.

However, since it’s not clear what makes the reading thread appear to do the right thing, the entire program runs on thin ice. The slightest change may break it. And the change doesn’t even need to be inside your program!.

Since your program is meant to hand over data from one thread to another, a SynchronousQueue would be a good choice. Unlike a real queue, it acts like a simple variable in the two thread scenario. But being a BlockingQueue, it solves the other flaws of your program as well, like polling a variable in a CPU consuming loop or possibly writing the same screenshot multiple times.

final SynchronousQueue<byte[]> data=new SynchronousQueue<>();

public void run(){
  while(continueLoop) try {
    data.put(getImageAsJPEG(robot.createScreenCapture(rectangle)));
    } catch (ImageFormatException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    } catch (InterruptedException ex) {
        if(continueLoop) ex.printStackTrace();
    }
}

public byte[] returnJPG() throws InterruptedException {
    return data.take();
}
Holger
  • 285,553
  • 42
  • 434
  • 765