On Tue, 1 Sep 2026 21:50:56 GMT, Andy Goryachev <[email protected]> wrote:
>> ## Summary
>>
>> This PR changes the behavior of `DataFormat` by allowing multiple instances
>> that contain the same set of mime types (ids).
>>
>>
>>
>> ## Problem
>>
>> There seems to be several issues with DataFormat API and implementation
>> discovered during review of the `Clipboard`-related code:
>>
>> 1. `static DataFormat::lookupMimeType(String)` is not thread safe: while
>> iterating over previously registered entries in the `DATA_FORMAT_LIST`
>> another thread might create a new instance (DataFormat L227)
>>
>> 2. `public DataFormat(String...)` constructor might throw an
>> `IllegalArgumentException` if one of the given mime types is already
>> assigned to another `DataFormat`. The origin of this requirement is unclear,
>> but one possible issue I can see is if the application has two libraries
>> that both attempt to create a `DataFormat` for let's say `"text/css"`. Then,
>> depending on the timing or the exact code path, an exception will be thrown
>> for which the library(-ies) might not be prepared. The constructor is also
>> not thread safe.
>>
>> 3. To avoid a situation mentioned in bullet 2, a developer would typically
>> call `lookupMimeType()` to obtain the already registered instance, followed
>> by a constructor call if such an instance has not been found. An example of
>> such code can be seen in webkit/UIClientImpl:299 - but even then, despite
>> that two-step process being synchronized, the code might still fail if *some
>> other* library or the application attempts to create a new instance of
>> DataFormat, since the constructor itself is not synchronized.
>>
>> 4. `DataFormat(new String[] { null })` is allowed but makes no sense!
>>
>> 5. The current implementation uses the `WeakReferenceQueue` which
>> theoretically might, under certain conditions, allow the application to
>> create mismatched `DataFormat`s.
>>
>> Why do we need to have the registry of previously created instances?
>> Unclear. My theory is that the DataFormat allows to have multiple mime-types
>> (ids) - example being `DataFormat.FILES = new
>> DataFormat("application/x-java-file-list", "java.file-list");` - and the
>> registry was added to prevent creation of a `DataFormat` with just one id
>> for some reason.
>>
>> Also, I could not find the origin of the multi-id requirement, or the origin
>> of the `java.file-list` id itself (it is not a valid mime type).
>>
>>
>>
>> ## Solution
>>
>> The proposed solution is to relax the constraint on the constructor to allow
>> multiple instances with the same set of mime types (ids). This might
>> require chan...
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
modules/javafx.graphics/src/test/java/test/javafx/scene/input/DataFormatTest.java
line 145:
> 143: @Test
> 144: public void concurrencyWithSingleID() throws Exception {
> 145: long duration = 5_000;
5 seconds for a single test is probably too long. Running all the tests in the
controls module takes about 2 minutes. I tested with 0.5 seconds and there's no
issue.
The way I do these race tests is with a `@RepeatedTest(M)` instead of the
sleep. Each tests runs N concurrent threads (N being "a lot") and the time it
takes them to finish is hardware-based, but running them M times
deterministically gives the non-flakiness guarantee. This way you don't need to
set a hardware-independent duration and the tests completes as fast as
possible. It's not like faster hardware requires more runs.
modules/javafx.graphics/src/test/java/test/javafx/scene/input/DataFormatTest.java
line 164:
> 162: }
> 163: try {
> 164: DataFormat f = new DataFormat(id);
`f` is unused, so no need for the LHS.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3912390946
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3912289442
