CODEWITHSUNDEEP
javamultithreadingswingthread-safety
herrtim
herrtim

Reputation: 2755

Best way to share object between threads?

I have a main Swing app with class members BufferedImage lastCapturedImage, ScheduledExecutorService executor with 2 threads in the thread pool.

BufferedImage lastCapturedImage;
ScheduledExecutorService executor = Executors.newScheduledThreadPool(2);
...
executor.scheduleWithFixedDelay(imageCaptureRunnable, 100, 1000 / TARGET_FPS, TimeUnit.MILLISECONDS);
executor.schedule(roboticArmRunnable, 500, TimeUnit.MILLISECONDS);

The first Runnable pulls images (BufferedImage) from a webcam and updates a class instance lastCapturedImage.

private final Runnable imageCaptureRunnable = new Runnable() {

  @Override
  public void run() {
    lastCapturedImage = webcam.getImage();
  }

};

The second Runnable processes the image and controls a robotic arm. The image capturing rate is much faster than the robotic arm consumer and only the most current image is needed by the robotic arm consumer. How best do I share the image in a thread safe way?

After researching this topic, my solution is to wrap the image (lastCapturedImage) in a synchronized block in the roboticArmRunnable's run() method, and make a copy of the image like this:

private final Runnable roboticArmRunnable = new Runnable() {

  @Override
  public void run() {
    while(true){
      BufferedImage clonedCameraCapture;
      synchronized (lastCapturedImage) {
        clonedCameraCapture = copyImage(lastCapturedImage);
      }
      // process the clonedCameraCapture image and move the robotic arm
    }
  }
};

My understanding is that the synchronized block will allow for the roboticArmRunnable to completely make the copy of the image before the imageCaptureRunnable is allowed to update the image. Am I doing this right??

Thanks in advance!

Upvotes: 2

Views: 1997

Answers (2)

JB Nizet
JB Nizet

Reputation: 691685

No, your code isn't safe.

First of all, for synchronization to be correct, all the access to the sharedstate must be synchronized, and not just the read access.

Second: synchronizing on a non-final field is wrong: since the field can change, one thread will acquire the lock on the old value and the second thread will then be able to enter the same synchronized section because the field has changed.

You don't have any atomicity problem to solve here: writing and reading a reference is guaranteed to be atomic. You have a visibility problem to solve though: nothing guarantees that the write made by the image reader thread (that writes the reference) will be visible by the robotic arm thread (that reads the reference).

So all you need is to make the field volatile, or to wrap it inside an AtomicReference:

private volatile BufferedImage lastImage;

or

private AtomicReference<BufferedImage> lastImageRef;

...
// in image reader
lastImageRef.set(theNewImage);

...
// in robotic arm
BufferedImage lastImage = lastImageRef.get();

If you were still willing to solve the visibility problem using synchronization, you would have to do something like:

static class LastImageHolder
    private BufferedImage lastImage;

    public synchronized BufferedImage get() {
        return lastImage;
    }

    public synchronized BufferedImage set(BufferedImage lastImage) {
        this.lastImage = lastImage;
    }
}

private LastImageHolder lastImageHolder = new LastImageHolder();

Upvotes: 4

OldCurmudgeon
OldCurmudgeon

Reputation: 65813

Functionality like this can be done without locks. Here's a OneOf:

class OneOf<T> {

    volatile int which = 0;
    final T[] them;

    public OneOf(T[] them) {
        this.them = them;
    }

    public T get() {
        return get(0);
    }

    public T get(int skip) {
        return them[(which + skip) % them.length];
    }

    public void skip() {
        which += 1;
        which %= them.length;
    }
}

You can now use get() to get the current one, get(1) to peek at the following one. In your case your image reader will choose it's image using get(1) to begin filling the next image while your robot will read the image using get() to get the current one.

Upvotes: -1

Related Questions