Implemented Double-Checked Locking in ImagehandleManager

[amartya4256]

This PR handles the creation of handles / cached ImageData per zoom for Image objects in a thread-safe way by the means of double-checked locking. The implementation extends the ImageHandleManager in the following way:

• ImageHandleManager#getHandleOrCreate now uses double-checked locking for thread-safety
• ImageHandleManager provides a method runSynchronized for to run a block of code synchronously. This is utilized while calling getImageData to ensure thread safety.

Depends on #2927
This fixes vi-eclipse/Eclipse-Platform#562

[HeikoKlare]

I really like the idea to wrap the handle-for-zoom mapping into a separate class. That clearly assigns all related management functionality to that class. I would be in favor of extracting the pure refactoring of the management functionality in a dedicated class into a separate PR. That PR will just improve code quality and comprehensibility and not improve thread safety.

You find several questions and proposals in the detailed comments. The probably most essential point is that to my understanding the implementation is not fully thread safe.

In addition, I am not sure if the concept of using a synchronized data structure is fully sufficient for all cases here (e.g., if we also include the rest of the API such as getBounds() and getImageData()). At least the complexity could increase such that ensuring correctness and maintainability becomes complex. Maybe we better see that when we adapt those methods as well, but I could imagine that simply synchronizing write access might be sufficient (as we don't expect multiple consumers to write for different zooms) and provides reduced compexity and error proneness.

[github-actions]

Test Results (win32)

   34 files  ±0     34 suites  ±0   3m 57s ⏱️ -55s
4 632 tests ±0  4 559 ✅ ±0  73 💤 ±0  0 ❌ ±0 
  167 runs  ±0    164 ✅ ±0   3 💤 ±0  0 ❌ ±0 

Results for commit 4da78e2. ± Comparison against base commit a026b13.

♻️ This comment has been updated with latest results.

[HeikoKlare]

As written here #2905 (review), getBounds() is also affected by potential race conditions.
I went through all call sequences of methods in ImageHandleManager and only found getBounds() with a potential remaining race condition. Still it would be good to have someone confirm this by checking the sequences.

In addition, I wonder why/if we need this runSynchronized method to synchronize on the map of ImageHandleManager or whether we cannot simply synchronize on ImageHandleManager itself? Then we would not need that method.
Edit: Maybe it would even be better to keep the synchronization inside ImageHandleManager but not make it explicit via an runSynchronized method but having a specific method for the use case that does the synchronizaiton, i.e., have something like a <R> R executeOnHandle(int zoom, Function<Optional<ImageHandle>,R> execution) method for the scenario(s) using runSynchronized like getting the image data.

[laeubi]

I have not looked into the full details here but using syncronized in SWT code looks wrong. The whole SWT code assumes it is always executed single threaded on the display thread, so where are different Threads are coming from here? If it is because of multiple Displays I think the cache should more be per display and avoid any complicate synchronization here.

[HeikoKlare]

The point is that resources used to be accessible from any thread (even though it is not recommended, but it worked and consumers used it) and this behavior broke when introducing the multi-handle support. This change is just restoring that capability.

[laeubi]

The point is that resources used to be accessible from any thread (even though it is not recommended, but it worked and consumers used it) and this behavior broke when introducing the multi-handle support

Looking at the Resource (and its subclasses) I can't find any sign neither in code nor API that it is safe to access these from different threads.

If we still want to support this I think synchronized primitives are not the right way and instead concurrent data-structures must be used instead and we are prone to deadlocks here.

[laeubi]

Suggested change

	private Map<Integer, DestroyableImageHandle> zoomLevelToImageHandle = new ConurrentHashMap<>();

[laeubi]

Use zoomLevelToImageHandle.computeIfAbsent(...) or compute(...) on the concurrent map instead of own synchronization primitives

[amartya4256]

93108dc7d4
That's how I pretty much started off earlier but since we also need to implement this for getImageData which either retrieves it from ImageHandle#getImageData or creates an ImageData and caches it; we needed different mechanism and overall it made sense to use a consistent locking mechanism rather than using 2 different approaches in 2 different places. That's why we decided to go with double-checked locking using synchronized.

[laeubi]

I must confess I don't understand the befit of this approach and why it should not work for getImageData.

Assume Thread 1 is entering here into the map.computeIfAbsent.

Now Thread 2 is entering getImageData and will be blocked on the get call until Thread 1 completes, what has the same effect as Thread 1 entering the synchronized and Thread 2 is hitting the null and then entering the synchronization and needs to wait until Thread 1 finishes.

The main difference here is, that if we have more threads then with your approach ALL threads will be blocked regardless of zoomlevel (both in getImageData and getOrCreate) even if they aim for different zoom levels!

... not talking even about the case that we have a race condition between two threads one that is removing it an one that is getting it (what will make a possibly destroyed image handle visible to the getting thread.

So actually I think one even needs to use Map.compute here and do two checks: If current value is null or current value is disposed we need to create a new handle...

[HeikoKlare]

why it should not work for getImageData.

getImageData() is not doing an atomic call on the ImageHandleManager, which would be necessary to make a synchronization via the data structure. Maybe there is some possibility to refactor the code such that it is wrapped into an atomic data structure access.

ALL threads will be blocked regardless of zoomlevel (both in getImageData and getOrCreate) even if they aim for different zoom levels

That's correct but not a problem at all. This synchronization is only necessary to avoid issues due to the extremely rare case of a race condition, so there is no necessity to ensure that access to the data structure with different zooms concurrently works in an efficient way.

[HeikoKlare]

Looking at the Resource (and its subclasses) I can't find any sign neither in code nor API that it is safe to access these from different threads.

That's true, there is nothing explicitly implemented to ensure thread-safety. However, most of the functionality was effectively thread-safe as after object initialization most of the state was effectively final (in particular the handles themselves), such that without any additional effort all access scenarios that did not modify state worked find in a concurrent access scenario.