[Win32] Thread-safe modification of Edge instances #1787
Conversation
HeikoKlare
force-pushed
the
edge_disposal_cme
branch
from
57ca43c to
dbdb74d
Compare
| private record WebViewEnvironment(ICoreWebView2Environment environment, List<Edge> instances) { | ||
| public WebViewEnvironment(ICoreWebView2Environment environment) { | ||
| this (environment, new ArrayList<>()); | ||
| this (environment, Collections.synchronizedList(new ArrayList<>())); |
There was a problem hiding this comment.
| this (environment, Collections.synchronizedList(new ArrayList<>())); | |
| this (environment, new CopyOnWriteArrayList<>()); |
I don'tsee a copy in the PR... but if you use |
HeikoKlare
force-pushed
the
edge_disposal_cme
branch
from
dbdb74d to
395eed5
Compare
The WebView environment used for Edge manages a list of all currently used Edge instances inside an application. This data structure is modified at different places, which may be executed by different threads, such introducing a risk of race conditions on the unsynchronized data structure. In addition, when disposing a Display (such as at application shutdown), Edge instances are disposed in an according dispose listener. This can cause ConcurrentModificationExceptions as the listener iterates through the list of Edge instances and calls their disposal, which in turn removes the Edge instance from that list. This change makes the data structure thread-safe and copy-on-write to avoid that ConcurrentModificationException occur when iterating over it while removing elements from it.
|
Good point, thank you! I pushed an incomplete state where the copy operatoin was missing. But using an copy-on-write data structure makes more sense anyway, as it does not require a local avoidance of concurrent modification when accessing the data structure but just solves that by the type of data structure itself, also making it easier to understand (as it's not immediately clear why there is a need for making a copy of the data structure). I updated the PR accordingly. |
|
Additional info: this is a stack trace of an exception that is to be avoided by this PR: |
| private static final Duration MAXIMUM_OPERATION_TIME = Duration.ofMillis(Integer.getInteger(WEB_VIEW_OPERATION_TIMEOUT, 5_000)); | ||
|
|
||
| private record WebViewEnvironment(ICoreWebView2Environment environment, ArrayList<Edge> instances) { | ||
| private record WebViewEnvironment(ICoreWebView2Environment environment, List<Edge> instances) { |
There was a problem hiding this comment.
I wonder if a record is really useful here. can the instances probably be part of ICoreWebView2Environment?
There was a problem hiding this comment.
ICoreWebView2Environment is a native interface (https://learn.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/icorewebview2environment?view=webview2-1.0.2957.106), so I guess nothing we could (or at least should) extend.
There was a problem hiding this comment.
okay than its not an option of course ...
|
Jenkins failures are known and unrelated (in particular since the change only affects Edge, which is not executed in the Jenkins build). |
2 participants
The WebView environment used for Edge manages a list of all currently used Edge instances inside an application. This data structure is modified at different places, which may be executed by different threads, such introducing a risk of race conditions on the unsynchronized data structure. In addition, when disposing a Display (such as at application shutdown), Edge instances are disposed in an according dispose listener. This can cause ConcurrentModificationExceptions as the listener iterates through the list of Edge instances and calls their disposal, which in turn removes the Edge instance from that list.
This change makes the data structure thread-safe and copy-on-write to avoid that ConcurrentModificationException occur when iterating over it while removing elements from it.