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 thread-safe compute methods
• ImageHandleManager#executeOnHandle also uses ConcurrentHashMap#compute for thread-safety. This is utilized while calling getImageData to ensure thread safety.

Depends on #2964
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   5m 16s ⏱️ +30s
4 636 tests ±0  4 563 ✅ ±0  73 💤 ±0  0 ❌ ±0 
  170 runs  ±0    167 ✅ ±0   3 💤 ±0  0 ❌ ±0 

Results for commit 96f133c. ± Comparison against base commit 200532a.

♻️ 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.

[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.

[HeikoKlare]

The code seems to be fine regarding what is supposed to be achieved. In general, the implementations based on the concurrent data structure looks much better/simpler than before. The more complex executeOnHandle call(s) can probably be simplified for better understanding, as currently those calls are quite hard to understand. I made a concrete proposal for that. With that, I think this is the best we can achieve, and we actually don't introduce any relevant cognitive overhead.

[HeikoKlare]

At a first glance, the code looks fine now. I will test tomorrow so that we can hopefully merge this for M2.

There was a comment about opportunities for larger refactorings of the Image class (which I do not find anymore). It's definitely true that we can keep refactoring, making the wrappers independent from the containing Image class, better handling the data caches etc. This may be done as follow-up work but always has to be done very carefully as with previous refactorings we often scenarios that require specific handling (such as uncommon autoscaling modes or the like) and probably not all relevant scenarios are (or maybe cannot even be) properly reflected by regression tests. So larger refactoring efforts would be something to proceed with for next M1.