8373452: DataFormat threading and API issues by andy-goryachev-oracle · Pull Request #2197 · openjdk/jfx

andy-goryachev-oracle · 2026-06-25T14:43:31Z

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:

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)
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.
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.
DataFormat(new String[] { null }) is allowed but makes no sense!
The current implementation uses the WeakReferenceQueue which theoretically might, under certain conditions, allow the application to create mismatched DataFormats.

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 changing the application code from identity compare (==) to .equals().

The change also adds synchronization on the registry inside the constructor and DataFormat::lookupMimeType(String).

Additionally, the constructor checks for null ids, throwing an IllegalArgumentException. (Quite possibly, we might want to disallow new DataFormat((String[])null) which is currently allowed.

Compatibility Impact

The applications that relied on identity check ( if(dataFormat == DataFormat.FILES) ) must be changed to use the equals() method. This might be a rare problem because most of the time the applications declare custom format as a static constant and use it to access the clipboard / dragboard data.

The constructor throws a NullPointerException if any of the ids is null.

Alternatives

One alternative is to deprecate the multi-id constructor and only allow one mime type per DataFormat, also removing the java.file-list id. We could then still keep Set<String> DataFormat.getIdentifiers() which will always return a set with one element.

Notes

This PR presents an alternative solution to 8373452: DataFormat threading and API issues #2006 (please refer to some very good comments there made by @mstr2 and @nlisker )
I think there is enough behavioral differences that a CSR is warranted.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8373452: DataFormat threading and API issues (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2197/head:pull/2197
$ git checkout pull/2197

Update a local copy of the PR:
$ git checkout pull/2197
$ git pull https://git.openjdk.org/jfx.git pull/2197/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2197

View PR using the GUI difftool:
$ git pr show -t 2197

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2197.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-06-25T14:43:54Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-06-25T14:44:48Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-06-25T17:56:04Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: rfr. This can be overridden with the /reviewers command.

mlbridge · 2026-06-25T17:59:13Z

Webrevs

nlisker

Added some suggestions, but haven't done a serious review.

nlisker · 2026-06-25T20:36:02Z

-                    throw new IllegalArgumentException("DataFormat '" + id +
-                            "' already exists.");
+        if (ids == null) {
+            this.identifier = Collections.<String>emptySet();


Suggested change

this.identifier = Collections.<String>emptySet();

this.identifier = Set.of()

nlisker · 2026-06-25T20:36:34Z

+            this.identifier = Collections.<String>emptySet();
+            // don't care about registry
+        } else {
+            this.identifier = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(ids)));


Suggested change

this.identifier = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(ids)));

this.identifier = Set.of(ids);

This also throws on nulls (but NPE, not IAE; you can rethrow). It also throws on duplicates which we need to do anyway.

nlisker · 2026-06-25T21:02:35Z

+                    if (id == null) {
+                        throw new IllegalArgumentException("DataFormat id must not be null.");
+                    }


Suggested change

if (id == null) {

throw new IllegalArgumentException("DataFormat id must not be null.");

}

the proposed code is no different? in any case, throwing an NPE might be better in this case.

I'm proposing to remove this since the null check in ids was done with Set.of(ids). It's unreachable code (unless ids is changed in a background thread maybe, but I don't think this is a real concern, and you can work with identifier instead anyway).

I agree (I think I got confused by the github diff in the conversation).

nlisker · 2026-06-25T21:12:10Z

+        } else if (obj instanceof DataFormat f) {
+            return identifier.equals(f.identifier);
        }
-
        return false;


Suggested change

} else if (obj instanceof DataFormat f) {

return identifier.equals(f.identifier);

}

return false;

}

return (obj instanceof DataFormat df && identifier.equals(df.identifier);

nlisker · 2026-06-25T21:36:02Z

+                    DataFormat f = registry.get(id);
+                    if (f != null) {
+                        if (!this.identifier.equals(f.identifier)) {
+                            throw new IllegalArgumentException("DataFormat '" + id + "' already exists.");
+                        }
+                        isNew = false;
+                    }


Suggested change

DataFormat f = registry.get(id);

if (f != null) {

if (!this.identifier.equals(f.identifier)) {

throw new IllegalArgumentException("DataFormat '" + id + "' already exists.");

}

isNew = false;

}

boolean noneExist = Collections.disjoint(this.identifie, registry.keySet());

my version is better as it avoids scanning twice

This claim might be too strong. From a performance perspective, the sizes of the collections are very small (a DataFormat will realistically have at most 3 ids and the total number of ids/mime-types that exist in the wild is probably in the dozens only), so performance is not really a consideration. However, if we do look at it, your implementation iterates over the array, which is O(n), with a hashed get which is O(1). Collections.disjoint is optimized to the given collections - it iterates over the smaller collection, which is O(n), and checks contains which is O(1). So I don't think your code gives better performance.

From a readability perspective, what your code says is "for each id, find a data format that uses it" (and if the DataFormat shares 2 or more ids, you will find and compare the same DataFormat twice or more). disjoint is clearer: "does any of the new ids exist already?"

In fact, I'm not sure why you're throwing only when not all the identifiers are the same, that is, why you specified "if one of the given ids is already assigned to another DataFormat with a different set of ids". If some are the same it's an exception, but if all of them are the same then it's not? Maybe I'm missing something.

my version is faster, but the difference is so minute it's irrelevant. I can use disjoint() here.

If some are the same it's an exception, but if all of them are the same then it's not?

This is the essence of the change. We are to allow multiple DataFormat instances if they are equal, so as to avoid the unnecessary exceptions.

The reason we can't allow disjoint formats is because the lookupMimeType() logic will break, and that would be a more drastic breaking change.

nlisker · 2026-06-26T16:31:17Z

+                    if (id == null) {
+                        throw new IllegalArgumentException("DataFormat id must not be null.");
+                    }


I'm proposing to remove this since the null check in ids was done with Set.of(ids). It's unreachable code (unless ids is changed in a background thread maybe, but I don't think this is a real concern, and you can work with identifier instead anyway).

nlisker · 2026-06-26T16:32:44Z

-                            "' already exists.");
+        if (ids == null) {
+            this.identifier = Set.of();
+            // don't care about registry


You can return here and avoid the big else branch.

I like big else branches :-)

nlisker · 2026-06-26T17:38:10Z

+                    DataFormat f = registry.get(id);
+                    if (f != null) {
+                        if (!this.identifier.equals(f.identifier)) {
+                            throw new IllegalArgumentException("DataFormat '" + id + "' already exists.");
+                        }
+                        isNew = false;
+                    }


This claim might be too strong. From a performance perspective, the sizes of the collections are very small (a DataFormat will realistically have at most 3 ids and the total number of ids/mime-types that exist in the wild is probably in the dozens only), so performance is not really a consideration. However, if we do look at it, your implementation iterates over the array, which is O(n), with a hashed get which is O(1). Collections.disjoint is optimized to the given collections - it iterates over the smaller collection, which is O(n), and checks contains which is O(1). So I don't think your code gives better performance.

From a readability perspective, what your code says is "for each id, find a data format that uses it" (and if the DataFormat shares 2 or more ids, you will find and compare the same DataFormat twice or more). disjoint is clearer: "does any of the new ids exist already?"

In fact, I'm not sure why you're throwing only when not all the identifiers are the same, that is, why you specified "if one of the given ids is already assigned to another DataFormat with a different set of ids". If some are the same it's an exception, but if all of them are the same then it's not? Maybe I'm missing something.

andy-goryachev-oracle · 2026-06-29T18:22:04Z

If some are the same it's an exception, but if all of them are the same then it's not? Maybe I'm missing something.

We've got this lookupMimeType() which is supposed to return the right multi-id data format, and it will break if one registers mismatched formats:

DataFormat f1 = new DataFormat("a", "b");
DataFormat f2 = new DataFormat("b", "c");

// say whaat?
var what = DataFormat.lookupMimeType("b");

A better solution would be to disallow multi-id formats and require the single format id to be a valid mime type, but that would be a more drastic change.

andy-goryachev-oracle · 2026-06-29T18:23:05Z

re: disjoint(): sorry, I don't want to deal with peculiarities of it (empty set etc.).

nlisker · 2026-06-30T02:18:49Z

-                if (lookupMimeType(id) != null) {
-                    throw new IllegalArgumentException("DataFormat '" + id +
-                            "' already exists.");
+        if (ids == null) {


Suggested change

if (ids == null) {

if (ids == null || ids.length == 0) {

I think we can shortcut an empty id list.

nlisker · 2026-06-30T02:34:42Z

re: disjoint(): sorry, I don't want to deal with peculiarities of it (empty set etc.).

What peculiarities? We are talking about sets of Strings. How are empty sets a problem? If a set has no elements then it is disjoint; the docs are clear "Returns true if the two specified collections have no elements in common.", so any empty set returns `true.

It's a suggestions to make the code clearer, ignore it if you want. As it is, it takes quite some effort to understand what the constructor does and it's mostly complexity for irrelevant optimizations (the registry will not be larger than a few dozen entries, and this is a stretch probably). I'd value readability here over saving a microsecond and write:

	public DataFormat(String... ids) {
		if (ids == null || ids.length == 0) {
			identifier = Set.of();
			return;
		}

		identifier = Set.of(ids);

		if (registry.containsValue(this)) {
			// all ids exist in an existing data format - no need to change registry
			return;
		}
		if (!Collections.disjoint(identifier, registry.keySet())) {
			// existing ids can't be overridden - throw
			throw new IllegalArgumentException("An id already exists");
		}
		// no existing ids - add
		identifier.forEach(id -> registry.put(id, this));
	}

I also find the name identifier confusing, I would use identifiers since it's a collection.

Maran23 · 2026-06-30T08:04:04Z

+    @Test
+    public void nullArrayIsAllowedForCompatibilityReasons() {
+        String[] mimes = null;
+        new DataFormat(mimes);


Suggested change

new DataFormat(mimes);

assertDoesNotThrow(() -> new DataFormat(mimes));

Maran23 · 2026-06-30T08:05:46Z

+        assertThrows(NullPointerException.class, () -> {
+            String mime = null;
+            new DataFormat(mime);
+        });


Suggested change

assertThrows(NullPointerException.class, () -> {

String mime = null;

new DataFormat(mime);

});

String mime = null;

assertThrows(NullPointerException.class, () -> {

new DataFormat(mime);

});

Recommendation is to always have one statement in the assertThrows / assertDoesNotThrows

andy-goryachev-oracle · 2026-06-30T15:10:21Z

re: disjoint

Collections.disjoint() returns true with two empty sets.

And you are right, it's rather unlikely for any DataFormat (except FILES) to contain more than one element, so we are basically bikeshedding.

The main question is whether this PR solves the problem sufficiently, or should we ban multi-id data formats altogether? I could not find an origin or any use of java.file-list id, it's not even a valid mime type.

bridgekeeper · 2026-07-29T00:20:46Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2026-07-29T15:20:49Z

/touch

openjdk · 2026-07-29T15:22:34Z

@andy-goryachev-oracle The pull request is being re-evaluated and the inactivity timeout has been reset.

data format

d7e3611

nulls

003b956

andy-goryachev-oracle marked this pull request as ready for review June 25, 2026 17:52

openjdk Bot added the rfr Ready for review label Jun 25, 2026

nlisker reviewed Jun 25, 2026

View reviewed changes

review comments

76b11d8

nlisker reviewed Jun 26, 2026

View reviewed changes

andy-goryachev-oracle added 2 commits June 29, 2026 11:23

review comments

e5bc1fd

Merge branch 'master' into 8373452.dataformat.2

a1316be

nlisker reviewed Jun 30, 2026

View reviewed changes

Maran23 reviewed Jun 30, 2026

View reviewed changes

review comments

5b9f886

Merge branch 'master' into 8373452.dataformat.2

ef48568

	this.identifier = Collections.<String>emptySet();
	this.identifier = Set.of()

	this.identifier = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(ids)));
	this.identifier = Set.of(ids);

	if (id == null) {
	throw new IllegalArgumentException("DataFormat id must not be null.");
	}

	new DataFormat(mimes);
	assertDoesNotThrow(() -> new DataFormat(mimes));

Uh oh!

Conversation

andy-goryachev-oracle commented Jun 25, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Compatibility Impact

Alternatives

Notes

Progress

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Jun 25, 2026

Uh oh!

openjdk Bot commented Jun 25, 2026

Uh oh!

openjdk Bot commented Jun 25, 2026

Uh oh!

mlbridge Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

nlisker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Jun 29, 2026

Uh oh!

andy-goryachev-oracle commented Jun 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlisker commented Jun 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridgekeeper Bot commented Jul 29, 2026

Uh oh!

andy-goryachev-oracle commented Jul 29, 2026

Uh oh!

openjdk Bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

andy-goryachev-oracle commented Jun 25, 2026 •

edited by openjdk Bot

Loading

mlbridge Bot commented Jun 25, 2026 •

edited

Loading

andy-goryachev-oracle commented Jun 30, 2026 •

edited

Loading