Skip to content

Make Image class thread safe #562

@HeikoKlare

Description

@HeikoKlare

Description
The Image class is currently not thread safe. When accessing an Image from multiple threads concurrently, race conditions lead to inconssitent behavior. For example, parallel win32_getHandle or getImageData call can lead to concurrency issues and exceptions. They access the zoom-to-handle map in a non-thread-safe way. One thread may find image data / handle for a requested zoom missing and started its creation and meanwhile another thread finds the image data / handle missing starting another creation. Depending on the kind of image (image data provider, plain image or the like), different paths are taken leading to different issues, e.g.:

  • A plain image may fail trying to scale existing data
  • An image with a data provider may try to add two handles for the same zoom, which throws an exception
  • An image based on a file / file name provider may in the second retrieval find the meanwhile created result of the first query and delete it for a new one (see ImageFileNameProviderWrapper#loadImageData).

Resources are created for a display, so one might assume that they are not supposed to be accesed from that display's thread, but actually this was never enforced and there is also no conceptual need to limit access to the display's thread.

Reproduction
A snippet to reproduce some of the issues together with some sleep calls to enforce unexpected results of the race conditions is provided in this branch: https://github.com/vi-eclipse/eclipse.platform.swt/tree/image-concurrent-demo

The contained snippet is:

	public static void main(final String[] args) {
		final Display display = new Display();
		try {
			concurrentPlainImage(display);
		} catch (Exception e)  {
			e.printStackTrace();
		}
		try {
			concurrentDataProvider(display);
		} catch (Exception e)  {
			e.printStackTrace();
		}
		display.dispose();
		System.exit(0);
	}

	public static void concurrentPlainImage(final Display display) throws InterruptedException, ExecutionException {
		Image image = new Image(display, 1, 1);
		Future<ImageData> future1 = Executors.newSingleThreadExecutor().submit(() -> image.getImageData());
		Thread.sleep(1000);
		Future<ImageData> future2 = Executors.newSingleThreadExecutor().submit(() -> image.getImageData());
		future1.get();
		future2.get();
	}

	private static void concurrentDataProvider(final Display display) throws InterruptedException, ExecutionException {
		ImageDataProvider provider = zoom -> new ImageLoader().load("src/org/eclipse/swt/snippets/eclipse16.png")[0];
		Image image = new Image(display, provider);
		Future<?> future1 = Executors.newSingleThreadExecutor().submit(() -> image.win32_getHandle(image, 100));
		Thread.sleep(1000);
		Future<?> future2 = Executors.newSingleThreadExecutor().submit(() -> image.win32_getHandle(image, 100));
		future1.get();
		future2.get();
	}

Expected Behavior
The Image class should be thread safe, so that in particular calls modifying the zoom-to-handle map, i.e., getImageData and win32_getHandle, don't have any race conditions. This should be done in a least intrusive way, i.e., with as fine-grained locks as possible. In particular, just marking affected methods as synchronized would not be an option to me. I would rather try to synchronize the parts in which actually handles / image data are created. For example, getImageData and getImageMetadata both check if a handle for the zoom already exist and only if that is not the case, locking could be applied.

Metadata

Metadata

Assignees

Labels

HiDPIA HiDPI-Related Issue or FeatureSWTIssue for SWTVectorIssues specifically relevant for Vector Informatik

Type

No type

Projects

Status

👀 In Review

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions