Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 15 Mar 2024 14:45:36 GMT, Alexey Ivanov wrote: >> I'm adding a regression test for >> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and >> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091); it's also a >> regression test for >> [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690). >> >> I referenced this test in PR #17462 in [this >> comment](https://github.com/openjdk/jdk/pull/17462#issuecomment-1914844026). >> I fine-tuned the test since that time. >> >> The test doesn't fail all the time without the fix for JDK-8323670, however, >> it fails sometimes. If you run the test several times, it will likely fail >> _without the fix_. >> >> For me, the test fails about 10 times from 40 runs in the CI. It fails on >> macOS more frequently than on Linux. >> >> When the test passes, it usually completes in 5 minutes. >> >> **How the test works** >> >> The test creates a temporary directory in the current directory and creates >> a number of files in it. (The number of files is controlled by >> `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the >> temporary directory. >> >> The test starts several _scanner_ threads, the number is controlled by >> `NUMBER_OF_THREADS`, which repeatedly call >> `fileChooser.rescanCurrentDirectory()`. This results in calling >> `BasicDirectoryModel.validateFileCache` which starts a background thread — >> "Basic L&F File Loading Thread" — to enumerate the files. >> >> A timer is used to create new files in the directory that the file chooser >> is using. >> >> After enumerating the files, the File Loading Thread posts an event to EDT. >> The event updates `fileCache` and fires a `ListDataEvent`. >> >> If the File Loading Thread is iterating over `fileCache` using `Iterator` >> (when `fileCache.subList` or `fileCache.equals` is running; or a new >> `Vector` instance is created from a `fileCache` or its sublist) and >> `fileCache` is being updated on EDT, then `ConcurrentModificationException` >> is thrown. >> >> On Linux and on _headless_ macOS, `ShellFolder.invoke` is executed in the >> caller, which makes it easier to reproduce the issue. Because of >> [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179), there are >> several File Loading Threads, which also helps to reproduce the issue. >> >> On _headful_ macOS, the `BasicDirectoryModel` is not used, so the test does >> not reproduce the issue. >> >> On Windows, the test does not fail or fails with `OutOfMemoryError`. It is >> because all the File Loading Threads are serialised on the COM thread, >> `ShellFolder.invoke` submits the task to the COM thread and waits for i... > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Suppress throwing exceptions while deleting files Looks good to me. - Marked as reviewed by tr (Committer). PR Review: https://git.openjdk.org/jdk/pull/18109#pullrequestreview-1972734171
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 29 Mar 2024 20:02:53 GMT, Alexey Ivanov wrote: >>> However, there are cases where instantiating and testing is safe on main >>> thread. >> >> That is my point, make it less safe - when the component is created on one >> thread:EDT and then for some reason accessed on a different thread, the >> rescanCurrentDirectory should still work. > >> > However, there are cases where instantiating and testing is safe on main >> > thread. >> >> That is my point, make it less safe - when the component is created on one >> thread:EDT and then for some reason accessed on a different thread, the >> rescanCurrentDirectory should still work. > > I got your idea! > > However, I still see no benefit for doing it. > > Yes, when `JFileChooser` is instantiated, it creates its UI delegates and the > default model, `BasicDirectoryModel`. The constructor of the model calls > `validateFileCache` which starts the background thread and will update > `fileCache`. > > It does not make the initialisation less thread-safe: the same two threads > are involved — `FilesLoader` and EDT. Yet there's no race yet… > > The `ConcurrentModificationException` is thrown when the background > `FilesLoader` is iterating over `fileCache` while EDT runs > `DoChangeContents.run` which adds elements to `fileCache` or removes elements > from it. > > By the time the `Scanner` threads start, a race becomes possible. If the > initial `FilesLoader` completes and posts the `DoChangeContents` object to > EDT, the `FilesLoader` threads created by the scanners will race… but > `fileCache` is empty initially, thus iterating over it is very quick. > > Therefore getting `ConcurrentModificationException` is unlikely until > `fileCache` contains a list of files. This state is reached later in the > timeline of the test. > > The initial `FilesLoader` thread that's started when `JFileChooser` is > instantiated plays a role too… Yet it's running concurrently with the > `Scanner` threads whether the `JFileChooser` object is created on the current > thread or on EDT. > > --- > > Taking the above into account, instantiating `JFileChooser` on EDT adds > complexity to the test but brings no benefits. ok, I agree with you. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1545858033
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 15 Mar 2024 14:45:36 GMT, Alexey Ivanov wrote: >> I'm adding a regression test for >> [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and >> [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091); it's also a >> regression test for >> [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690). >> >> I referenced this test in PR #17462 in [this >> comment](https://github.com/openjdk/jdk/pull/17462#issuecomment-1914844026). >> I fine-tuned the test since that time. >> >> The test doesn't fail all the time without the fix for JDK-8323670, however, >> it fails sometimes. If you run the test several times, it will likely fail >> _without the fix_. >> >> For me, the test fails about 10 times from 40 runs in the CI. It fails on >> macOS more frequently than on Linux. >> >> When the test passes, it usually completes in 5 minutes. >> >> **How the test works** >> >> The test creates a temporary directory in the current directory and creates >> a number of files in it. (The number of files is controlled by >> `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the >> temporary directory. >> >> The test starts several _scanner_ threads, the number is controlled by >> `NUMBER_OF_THREADS`, which repeatedly call >> `fileChooser.rescanCurrentDirectory()`. This results in calling >> `BasicDirectoryModel.validateFileCache` which starts a background thread — >> "Basic L&F File Loading Thread" — to enumerate the files. >> >> A timer is used to create new files in the directory that the file chooser >> is using. >> >> After enumerating the files, the File Loading Thread posts an event to EDT. >> The event updates `fileCache` and fires a `ListDataEvent`. >> >> If the File Loading Thread is iterating over `fileCache` using `Iterator` >> (when `fileCache.subList` or `fileCache.equals` is running; or a new >> `Vector` instance is created from a `fileCache` or its sublist) and >> `fileCache` is being updated on EDT, then `ConcurrentModificationException` >> is thrown. >> >> On Linux and on _headless_ macOS, `ShellFolder.invoke` is executed in the >> caller, which makes it easier to reproduce the issue. Because of >> [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179), there are >> several File Loading Threads, which also helps to reproduce the issue. >> >> On _headful_ macOS, the `BasicDirectoryModel` is not used, so the test does >> not reproduce the issue. >> >> On Windows, the test does not fail or fails with `OutOfMemoryError`. It is >> because all the File Loading Threads are serialised on the COM thread, >> `ShellFolder.invoke` submits the task to the COM thread and waits for i... > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Suppress throwing exceptions while deleting files Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18109#pullrequestreview-1970499628
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 29 Mar 2024 02:00:18 GMT, Sergey Bylokhov wrote: > > However, there are cases where instantiating and testing is safe on main > > thread. > > That is my point, make it less safe - when the component is created on one > thread:EDT and then for some reason accessed on a different thread, the > rescanCurrentDirectory should still work. I got your idea! However, I still see no benefit for doing it. Yes, when `JFileChooser` is instantiated, it creates its UI delegates and the default model, `BasicDirectoryModel`. The constructor of the model calls `validateFileCache` which starts the background thread and will update `fileCache`. It does not make the initialisation less thread-safe: the same two threads are involved — `FilesLoader` and EDT. Yet there's no race yet… The `ConcurrentModificationException` is thrown when the background `FilesLoader` is iterating over `fileCache` while EDT runs `DoChangeContents.run` which adds elements to `fileCache` or removes elements from it. By the time the `Scanner` threads start, a race becomes possible. If the initial `FilesLoader` completes and posts the `DoChangeContents` object to EDT, the `FilesLoader` threads created by the scanners will race… but `fileCache` is empty initially, thus iterating over it is very quick. Therefore getting `ConcurrentModificationException` is unlikely until `fileCache` contains a list of files. This state is reached later in the timeline of the test. The initial `FilesLoader` thread that's started when `JFileChooser` is instantiated plays a role too… Yet it's running concurrently with the `Scanner` threads whether the `JFileChooser` object is created on the current thread or on EDT. --- Taking the above into account, instantiating `JFileChooser` on EDT adds complexity to the test but brings no benefits. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1544825191
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Wed, 27 Mar 2024 20:19:41 GMT, Alexey Ivanov wrote: > However, there are cases where instantiating and testing is safe on main > thread. That is my point, make it less safe - when the component is created on one thread:EDT and then for some reason accessed on a different thread, the rescanCurrentDirectory should still work. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1543954407
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 22 Mar 2024 21:45:36 GMT, Alexey Ivanov wrote: >>>Instantiating JFileChooser on EDT adds complexity for no benefit. The >>>JFileChooser is still accessed concurrently from several threads, including >>>Swing internal thread. >> >> But still, this is a common requirement to create UI components on EDT, and >> that will replicate more or less how it will work in the application. > >> > Instantiating JFileChooser on EDT adds complexity for no benefit. The >> > JFileChooser is still accessed concurrently from several threads, >> > including Swing internal thread. >> >> But still, this is a common requirement to create UI components on EDT, and >> that will replicate more or less how it will work in the application. > > And it is a common requirement ***not** to call > `fileChooser.rescanCurrentDirectory` concurrently* in 5 different threads. So > it does not apply. In general, I agree all Swing components are to be created and accessed on EDT. However, there are cases where instantiating and testing is safe on main thread. It is when the component is not shown on the screen, its state does not depend on any events dispatched by the event queue. There are such tests in the JDK code base. I think this test is one of such cases where `JFileChooser` can be (safely) created on the current thread. The test does not display the file chooser, its state does not depend on receiving input events and handling them. Essentially, the test verifies that *the implementation of `JFileChooser.rescanCurrentDirectory` is thread-safe*. It wasn't. Two bugs were resolved to make the implementation thread-safe: [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179). - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1541975141
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 22 Mar 2024 21:05:09 GMT, Sergey Bylokhov wrote: > > Instantiating JFileChooser on EDT adds complexity for no benefit. The > > JFileChooser is still accessed concurrently from several threads, including > > Swing internal thread. > > But still, this is a common requirement to create UI components on EDT, and > that will replicate more or less how it will work in the application. And it is a common requirement ***not** to call `fileChooser.rescanCurrentDirectory` concurrently* in 5 different threads. So it does not apply. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1536253490
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Tue, 19 Mar 2024 17:03:25 GMT, Alexey Ivanov wrote: >Instantiating JFileChooser on EDT adds complexity for no benefit. The >JFileChooser is still accessed concurrently from several threads, including >Swing internal thread. But still, this is a common requirement to create UI components on EDT, and that will replicate more or less how it will work in the application. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1536222501
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 15 Mar 2024 11:17:08 GMT, Alexey Ivanov wrote:
>> test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java
>> line 211:
>>
>>> 209: private static void deleteFile(final Path file) {
>>> 210: try {
>>> 211: Files.delete(file);
>>
>> Should we try to delete all files first and then throw an exception if any?
>
> I do try to remove all the files:
>
> https://github.com/openjdk/jdk/blob/0d6be7a4e4368ea67382af4321b9483236276e5a/test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java#L115-L120
>
> https://github.com/openjdk/jdk/blob/0d6be7a4e4368ea67382af4321b9483236276e5a/test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java#L202-L215
>
> Probably, this can be chained via `handleException`. On the other hand, I
> don't want to fail the test if cleaning up the files fails. However, now an
> exception thrown from the `finally` block will override any exception thrown
> from the test code, which is a bad thing, so I'll need to reconsider
> exception handling during clean-up.
>
> If test fails to clean up the files it created, jtreg will remove them. It is
> the reason why I use the current directory as the base, which is the
> `scratch` directory when run in jtreg.
I updated the code to avoid throwing `IOException` from cleaning up code. If an
exception occurs, its stack trace is printed to `stderr`. The test will not
fail if cleaning up the files fails.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1530772076
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Tue, 19 Mar 2024 01:08:25 GMT, Sergey Bylokhov wrote: >> I don't think it matters here… The test does not rely on even processing on >> EDT, no state of `JFileChooser` is modified except for calling >> `rescanCurrentDirectory`. Technically, I am not allowed to do so either. But >> if I call `rescanCurrentDirectory` from EDT only, the test becomes void. > > I think it will fire the property change events via firePropertyChange from > the constructor and it is better to do that on EDT. I guess it does fire property change events on the current thread rather than EDT in this case. I still don't think it matters in this case. After the `JFileChooser` is created, it is accessed on the Scanner threads only, and only to call its `rescanCurrentDirectory` method. This call, in its turn, creates a new background thread which accesses the fields and methods of `JFileChooser` as well as `FileSystemView`. Instantiating `JFileChooser` on EDT adds complexity for no benefit. The `JFileChooser` is still accessed concurrently from several threads, including Swing internal thread. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1530766002
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
On Fri, 15 Mar 2024 12:02:43 GMT, Alexey Ivanov wrote: >> test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java >> line 99: >> >>> 97: createFiles(temp); >>> 98: >>> 99: final JFileChooser fc = new JFileChooser(temp.toFile()); >> >> it should be created on EDT, just in case. > > I don't think it matters here… The test does not rely on even processing on > EDT, no state of `JFileChooser` is modified except for calling > `rescanCurrentDirectory`. Technically, I am not allowed to do so either. But > if I call `rescanCurrentDirectory` from EDT only, the test becomes void. I think it will fire the property change events via firePropertyChange from the constructor and it is better to do that on EDT. - PR Review Comment: https://git.openjdk.org/jdk/pull/18109#discussion_r1529545568
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel [v3]
> I'm adding a regression test for > [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and > [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091); it's also a > regression test for > [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690). > > I referenced this test in PR #17462 in [this > comment](https://github.com/openjdk/jdk/pull/17462#issuecomment-1914844026). > I fine-tuned the test since that time. > > The test doesn't fail all the time without the fix for JDK-8323670, however, > it fails sometimes. If you run the test several times, it will likely fail > _without the fix_. > > For me, the test fails about 10 times from 40 runs in the CI. It fails on > macOS more frequently than on Linux. > > When the test passes, it usually completes in 5 minutes. > > **How the test works** > > The test creates a temporary directory in the current directory and creates a > number of files in it. (The number of files is controlled by > `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the > temporary directory. > > The test starts several _scanner_ threads, the number is controlled by > `NUMBER_OF_THREADS`, which repeatedly call > `fileChooser.rescanCurrentDirectory()`. This results in calling > `BasicDirectoryModel.validateFileCache` which starts a background thread — > "Basic L&F File Loading Thread" — to enumerate the files. > > A timer is used to create new files in the directory that the file chooser is > using. > > After enumerating the files, the File Loading Thread posts an event to EDT. > The event updates `fileCache` and fires a `ListDataEvent`. > > If the File Loading Thread is iterating over `fileCache` using `Iterator` > (when `fileCache.subList` or `fileCache.equals` is running; or a new `Vector` > instance is created from a `fileCache` or its sublist) and `fileCache` is > being updated on EDT, then `ConcurrentModificationException` is thrown. > > On Linux and on _headless_ macOS, `ShellFolder.invoke` is executed in the > caller, which makes it easier to reproduce the issue. Because of > [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179), there are several > File Loading Threads, which also helps to reproduce the issue. > > On _headful_ macOS, the `BasicDirectoryModel` is not used, so the test does > not reproduce the issue. > > On Windows, the test does not fail or fails with `OutOfMemoryError`. It is > because all the File Loading Threads are serialised on the COM thread, > `ShellFolder.invoke` submits the task to the COM thread and waits for it to > complete. The chance of updating `fileCache` whil... Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Suppress throwing exceptions while deleting files - Changes: - all: https://git.openjdk.org/jdk/pull/18109/files - new: https://git.openjdk.org/jdk/pull/18109/files/0dfff701..73ea5bf4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18109&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18109&range=01-02 Stats: 15 lines in 1 file changed: 13 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18109.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18109/head:pull/18109 PR: https://git.openjdk.org/jdk/pull/18109
