diff --git a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java index 522c9fe3912..1680d116c79 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java @@ -19,6 +19,7 @@ import java.io.*; import java.util.*; import java.util.Map.*; +import java.util.concurrent.*; import java.util.function.*; import java.util.stream.*; @@ -138,7 +139,7 @@ public final class Image extends Resource implements Drawable { private final ImageHandleManager imageHandleManager = new ImageHandleManager(); private class ImageHandleManager { - private Map zoomLevelToImageHandle = new HashMap<>(); + private final ConcurrentMap zoomLevelToImageHandle = new ConcurrentHashMap<>(); InternalImageHandle get(int zoom) { final DestroyableImageHandle imageHandle = zoomLevelToImageHandle.get(zoom); @@ -154,14 +155,7 @@ InternalImageHandle getOrCreate(int zoom, Supplier creat return null; } - DestroyableImageHandle imageHandle = (DestroyableImageHandle) get(zoom); - if (imageHandle != null) { - return imageHandle; - } - - imageHandle = creator.get(); - zoomLevelToImageHandle.put(zoom, imageHandle); - return imageHandle; + return zoomLevelToImageHandle.computeIfAbsent(zoom, k -> creator.get()); } boolean contains(int zoom) { @@ -187,7 +181,6 @@ void destroyHandles(Predicate filter) { if (filter.test(zoomToHandle.getKey())) { DestroyableImageHandle imageHandle = zoomToHandle.getValue(); it.remove(); - zoomLevelToImageHandle.remove(imageHandle.zoom(), imageHandle); imageHandle.destroy(); } } @@ -2139,7 +2132,7 @@ protected ElementAtZoom loadImageData(int zoom) { } private abstract class ImageFromImageDataProviderWrapper extends AbstractImageProviderWrapper { - private final Map cachedImageData = new HashMap<>(); + private final ConcurrentMap cachedImageData = new ConcurrentHashMap<>(); void initImage() { @@ -2423,7 +2416,7 @@ public boolean equals(Object otherProvider) { } private abstract class BaseImageProviderWrapper extends DynamicImageProviderWrapper { - private final Map cachedImageData = new HashMap<>(); + private final ConcurrentMap cachedImageData = new ConcurrentHashMap<>(); protected final T provider; diff --git a/examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet_ImageHandleRaceCondition.java b/examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet_ImageHandleRaceCondition.java new file mode 100644 index 00000000000..5ab89e7d82d --- /dev/null +++ b/examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet_ImageHandleRaceCondition.java @@ -0,0 +1,168 @@ +/******************************************************************************* + * Copyright (c) 2026 Contributors and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Contributors - initial API and implementation + *******************************************************************************/ +package org.eclipse.swt.snippets; + +import java.util.concurrent.*; +import java.util.concurrent.atomic.*; +import org.eclipse.swt.SWT; +import org.eclipse.swt.graphics.*; +import org.eclipse.swt.widgets.Display; + +/** + * Snippet to demonstrate potential race condition in ImageHandleManager. + * + * This snippet creates multiple threads that concurrently access the same Image + * at the same zoom level, which could trigger the race condition in + * ImageHandleManager.getOrCreate() if it's not properly synchronized. + * + * The race condition occurs when: + * 1. Thread A calls getOrCreate(zoom) - checks if handle exists (returns null) + * 2. Thread B calls getOrCreate(zoom) - checks if handle exists (returns null) + * 3. Thread A creates new handle via creator.get() + * 4. Thread B creates ANOTHER handle via creator.get() (wasteful!) + * 5. Both threads put() their handles, one overwrites the other (resource leak!) + * + * For instructions on how to use this snippet, see: + * http://www.eclipse.org/swt/ + */ +public class Snippet_ImageHandleRaceCondition { + + private static final int NUM_THREADS = 20; + private static final int ITERATIONS_PER_THREAD = 100; + private static final int TARGET_ZOOM = 150; + + // Track handle creation to detect duplicate creations + private static final AtomicInteger handleCreationCount = new AtomicInteger(0); + private static final ConcurrentHashMap handleCreationsByZoom = new ConcurrentHashMap<>(); + + public static void main(String[] args) throws Exception { + Display display = new Display(); + + System.out.println("=== ImageHandleManager Race Condition Test ==="); + System.out.println("This test attempts to trigger the race condition by having"); + System.out.println("multiple threads concurrently access the same Image at the same zoom level.\n"); + + // Create an ImageDataProvider that tracks handle creations + ImageDataProvider provider = new ImageDataProvider() { + @Override + public ImageData getImageData(int zoom) { + // Track each time we're asked to create image data for a zoom level + int count = handleCreationsByZoom.compute(zoom, (k, v) -> v == null ? 1 : v + 1); + handleCreationCount.incrementAndGet(); + + // Add a small delay to increase chance of race condition + try { + Thread.sleep(1); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + + // Create image data + int size = 16 * zoom / 100; + ImageData imageData = new ImageData(size, size, 24, + new PaletteData(0xFF, 0xFF00, 0xFF0000)); + + // Fill with a simple pattern + for (int y = 0; y < size; y++) { + for (int x = 0; x < size; x++) { + imageData.setPixel(x, y, (x + y) % 2 == 0 ? 0xFFFFFF : 0x000000); + } + } + + if (count > 1) { + System.err.println("WARNING: Image data requested " + count + " times for zoom " + zoom + + " (possible race condition!)"); + } + + return imageData; + } + }; + + Image image = new Image(display, provider); + + // Force initial creation at 100% zoom + image.getImageData(100); + handleCreationsByZoom.clear(); + handleCreationCount.set(0); + + System.out.println("Starting concurrent access test with " + NUM_THREADS + " threads...\n"); + + // Create thread pool + ExecutorService executor = Executors.newFixedThreadPool(NUM_THREADS); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(NUM_THREADS); + + // Submit tasks that will all try to access the image at the same zoom level + for (int i = 0; i < NUM_THREADS; i++) { + final int threadId = i; + executor.submit(() -> { + try { + // Wait for all threads to be ready + startLatch.await(); + + // Each thread tries to get the image handle multiple times + for (int j = 0; j < ITERATIONS_PER_THREAD; j++) { + display.syncExec(() -> { + try { + // This should trigger getOrCreate() in ImageHandleManager + @SuppressWarnings("unused") + ImageData data = image.getImageData(TARGET_ZOOM); + } catch (Exception e) { + System.err.println("Thread " + threadId + " iteration " + j + " failed: " + e.getMessage()); + e.printStackTrace(); + } + }); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + doneLatch.countDown(); + } + }); + } + + // Start all threads at once + startLatch.countDown(); + + // Wait for completion + doneLatch.await(); + executor.shutdown(); + executor.awaitTermination(30, TimeUnit.SECONDS); + + // Report results + System.out.println("\n=== Test Results ==="); + System.out.println("Total image data creations: " + handleCreationCount.get()); + System.out.println("Creations by zoom level:"); + handleCreationsByZoom.forEach((zoom, count) -> + System.out.println(" Zoom " + zoom + ": " + count + " creation(s)")); + + int creationsForTargetZoom = handleCreationsByZoom.getOrDefault(TARGET_ZOOM, 0); + if (creationsForTargetZoom > 1) { + System.err.println("\n❌ RACE CONDITION DETECTED!"); + System.err.println("Image data was created " + creationsForTargetZoom + + " times for zoom " + TARGET_ZOOM); + System.err.println("Expected: 1 creation"); + System.err.println("This indicates that multiple threads created duplicate handles."); + } else if (creationsForTargetZoom == 1) { + System.out.println("\n✓ No race condition detected (or it didn't manifest in this run)"); + System.out.println("Note: Race conditions are non-deterministic; absence of detection"); + System.out.println("doesn't guarantee the code is thread-safe."); + } else { + System.out.println("\n⚠ Zoom level " + TARGET_ZOOM + " was never accessed"); + } + + image.dispose(); + display.dispose(); + } +} diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java index 47548773193..79c445e59f8 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java +++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java @@ -1183,5 +1183,122 @@ public void test_gcOnImageGcDrawer_imageDataAtNonDeviceZoom() { } } +/** + * Test for race condition fix in ImageHandleManager. + * This test verifies that concurrent access to Image.getImageData() at the same zoom level + * doesn't create duplicate handles or cause resource leaks. + * + * The fix uses ConcurrentHashMap instead of HashMap to ensure thread-safe access. + */ +@Test +public void test_ConcurrentImageHandleAccess() throws Exception { + final int NUM_THREADS = 10; + final int ITERATIONS_PER_THREAD = 50; + final int TARGET_ZOOM = 150; + + // Track how many times image data is created for each zoom level + final AtomicInteger creationCount = new AtomicInteger(0); + final java.util.concurrent.ConcurrentHashMap creationsByZoom = new java.util.concurrent.ConcurrentHashMap<>(); + + // Create an ImageDataProvider that tracks handle creations + ImageDataProvider testProvider = new ImageDataProvider() { + @Override + public ImageData getImageData(int zoom) { + // Track creation attempts + creationsByZoom.computeIfAbsent(zoom, k -> new AtomicInteger(0)).incrementAndGet(); + creationCount.incrementAndGet(); + + // Create image data + int size = 16 * zoom / 100; + ImageData imageData = new ImageData(size, size, 24, + new PaletteData(0xFF, 0xFF00, 0xFF0000)); + + // Fill with a simple pattern + for (int y = 0; y < size; y++) { + for (int x = 0; x < size; x++) { + imageData.setPixel(x, y, (x + y) % 2 == 0 ? 0xFFFFFF : 0x000000); + } + } + + return imageData; + } + }; + + Image testImage = new Image(display, testProvider); + + try { + // Force initial creation at 100% zoom to initialize the image + testImage.getImageData(100); + + // Reset counters + creationsByZoom.clear(); + creationCount.set(0); + + // Create threads that will concurrently access the image + java.util.concurrent.ExecutorService executor = java.util.concurrent.Executors.newFixedThreadPool(NUM_THREADS); + java.util.concurrent.CountDownLatch startLatch = new java.util.concurrent.CountDownLatch(1); + java.util.concurrent.CountDownLatch doneLatch = new java.util.concurrent.CountDownLatch(NUM_THREADS); + final java.util.concurrent.atomic.AtomicReference errorRef = new java.util.concurrent.atomic.AtomicReference<>(); + + // Submit tasks that will all try to access the image at the same zoom level + for (int i = 0; i < NUM_THREADS; i++) { + executor.submit(() -> { + try { + // Wait for all threads to be ready + startLatch.await(); + + // Each thread tries to get the image data multiple times + for (int j = 0; j < ITERATIONS_PER_THREAD; j++) { + display.syncExec(() -> { + try { + // This should trigger getOrCreate() in ImageHandleManager + ImageData data = testImage.getImageData(TARGET_ZOOM); + assertNotNull(data, "ImageData should not be null"); + } catch (Throwable e) { + errorRef.compareAndSet(null, e); + } + }); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + errorRef.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }); + } + + // Start all threads at once + startLatch.countDown(); + + // Wait for completion + assertTrue(doneLatch.await(30, java.util.concurrent.TimeUnit.SECONDS), + "Test threads should complete within timeout"); + + executor.shutdown(); + assertTrue(executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS), + "Executor should shutdown within timeout"); + + // Check if any errors occurred + Throwable error = errorRef.get(); + if (error != null) { + throw new AssertionError("Error during concurrent access", error); + } + + // Verify that image data was created exactly once for the target zoom + // (With the fix, ConcurrentHashMap.computeIfAbsent ensures single creation) + int creationsForTargetZoom = creationsByZoom.getOrDefault(TARGET_ZOOM, new AtomicInteger(0)).get(); + + // With the race condition fix, we expect exactly 1 creation for the target zoom + // Without the fix, multiple threads might create duplicate handles + assertEquals(1, creationsForTargetZoom, + "Image data should be created exactly once for zoom " + TARGET_ZOOM + + " (race condition would cause multiple creations)"); + + } finally { + testImage.dispose(); + } +} + }