8340404: CharsetProvider specification updates #21076

justin-curtis-lu · 2024-09-19T00:07:30Z

Please review this PR and CSR which clarifies that a CharsetProvider SPI can be provided via the application module path.

This came as a result from a comment on the PR of JDK-8339735.

I included some minor wording on deploying as a module, with a link to Service Loader's Developing Service Providers section to handle the heavy lifting. However, I am happy to adjust as needed, if it is felt that more info should be laid out directly on CharsetProvider's class description itself.

An associated test is provided to confirm that a CharsetProvider can be deployed as a module.

Additionally some outdated info is corrected as part of this change.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8340412 to be approved

Issues

JDK-8340404: CharsetProvider specification updates (Bug - P4)
JDK-8340412: CharsetProvider specification updates (CSR)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21076

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21076.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-19T00:08:00Z

👋 Welcome back jlu! 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 · 2024-09-19T00:08:06Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8340404: CharsetProvider specification updates

Reviewed-by: alanb, naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 115 new commits pushed to the master branch:

1bc13a1: 8340552: Harden TzdbZoneRulesCompiler against missing zone names
e6373b5: 8340679: Misc tests fail assert(!set || SafepointSynchronize::is_at_safepoint()) failed: set once or at safepoint
2349bb7: 8340974: Ambiguous name of jtreg property vm.libgraal.enabled
5d062e2: 8340576: Some JVMCI flags are inconsistent
1447967: 8339261: Logs truncated in test javax/net/ssl/DTLS/DTLSRehandshakeTest.java
a02d895: 8333403: Write a test to check various components events are triggered properly
bb040ef: 8340983: Use index and definition tags in Object and Double
8225a5f: 8340981: Update citations to "Hacker's Delight"
aceae76: 8339460: CDS error when module is located in a directory with space in the name
aeaa4f7: 8336895: BufferedReader doesn't read full \r\n line ending when it doesn't fit in buffer
... and 105 more: https://git.openjdk.org/jdk/compare/bc36ace72c1189dcd6d0c05d40d8c568acd89b01...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-09-19T00:08:49Z

@justin-curtis-lu The following label will be automatically applied to this pull request:

nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-09-19T00:13:03Z

Webrevs

AlanBateman · 2024-09-19T05:39:39Z

test/jdk/java/nio/charset/spi/provider/module-info.java

+ * questions.
+ */
+module provider {
+    exports spi;


It's a bit of anti pattern for service provider modules to export an API, can you drop this? I don't think it's possible to check the test setup while that is there.

Thanks for taking a look Alan. Your test comments should be addressed in 790abd0.
Did you have any comments on the current doc change?

Did you have any comments on the current doc change?

I've been slow to reply on that because I'm not 100% sure if the spec is correct. Can you double check that the CCL is actually used. I think it uses the system class loader, and maybe the platform class loader for the extended charsets.

Hi Alan,

I think you're right that the CCL is not used.

The standard charsets are loaded via the SystemClassLoader, the extended are loaded via the platformClassLoader, and BazProvider does appear to be loaded via the SystemClassLoader. See below,

// Assert default context class loader is the system class loader Thread.currentThread().getContextClassLoader(); // default context class loader -> jdk.internal.loader.ClassLoaders$AppClassLoader@1d81eb93 ClassLoader.getSystemClassLoader(); // system class loader -> jdk.internal.loader.ClassLoaders$AppClassLoader@1d81eb93 // Change the default context class loader Thread.currentThread().setContextClassLoader(new FooLoader()); // Change default context class loader to not be the system // Check the class loaders for the various Charsets Charset.availableCharsets().get("utf-8").getClassLoader(); // standard -> null Charset.availableCharsets().get("Big5").getClassLoader(); // extended -> jdk.internal.loader.ClassLoaders$PlatformClassLoader@55a95ab1 Charset.availableCharsets().get("BAZ").getClassLoader(); // provider -> jdk.internal.loader.ClassLoaders$AppClassLoader@1d81eb93 Thread.currentThread().getContextClassLoader(); // context class loader -> CharsetProviderAsModuleTest$FooLoader@1d15c6cc

Can we simply omit the line:

"Charset providers are looked up via the current thread's context class loader".

Thanks for checking. I remember JDK-4619777 and it's possible there are one or two other JBS issue on the topic. It may be been interesting to include a charset provider with a dynamically loaded application at one point but seems less interesting in 2024. So I think we should just update the spec to align with the long standing behavior, meaning specify that the charset provider must be visible to the system class loader. I think "by some other platform-specific means" by can be dropped too.

Updated in d90f6ca.

AlanBateman · 2024-09-19T05:40:45Z

test/jdk/java/nio/charset/spi/CharsetProviderAsModuleTest.java

+            throw new RuntimeException("SPI BazProvider did not provide BAZ Charset");
+        } else {
+            bazCs = cs.get("BAZ");
+            var aliases = bazCs.aliases();


I think this needs to test that bazCs.getClass().getModule() is a named module.

AlanBateman · 2024-09-19T05:48:30Z

test/jdk/java/nio/charset/spi/CharsetProviderAsModuleTest.java

+ * @bug 8340404
+ * @summary Check that a CharsetProvider SPI can be deployed as a module
+ * @modules provider
+ * @build provider/module-info provider/spi.BazProvider


You can use @build provider/* here and jtreg will do the right thing.

AlanBateman · 2024-09-19T05:49:19Z

test/jdk/java/nio/charset/spi/CharsetProviderAsModuleTest.java

+ * @test
+ * @bug 8340404
+ * @summary Check that a CharsetProvider SPI can be deployed as a module
+ * @modules provider


The @modules tag is used for test selection, I don't know what L28 does.

openjdk · 2024-09-19T16:51:43Z

@justin-curtis-lu this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8340404-CharsetProvider-asModule
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

naotoj · 2024-09-19T16:58:44Z

src/java.base/share/classes/java/nio/charset/spi/CharsetProvider.java

@@ -36,14 +36,28 @@
 * zero-argument constructor and some number of associated charset
 * implementation classes.  Charset providers may be installed in an instance
 * of the Java platform as extensions.  Providers may also be made available by


It is not related to module path, but taking this opportunity, let's amend this sentence that refers to "extensions" which is obsolete.

AlanBateman · 2024-09-20T13:07:05Z

src/java.base/share/classes/java/nio/charset/spi/CharsetProvider.java

- * loader}.
+ * loader}. See {@link java.util.ServiceLoader##developing-service-providers
+ * Deploying Service Providers} for further detail on deploying a charset
+ * provider as a module or on the class path.


I think it would better to say "deployed" rather than "available" here, that will align with the later text.

I presume you meant the "available" on line 37 should be "deployed" and changed as such.

AlanBateman · 2024-09-22T07:48:03Z

src/java.base/share/classes/java/nio/charset/spi/CharsetProvider.java

- * loader}.
+ * zero-argument constructor and some number of associated {@code Charset}
+ * implementation classes.  Charset providers are deployed by adding them to either
+ * the application module path or the application class path. In order to be looked


The latest update to the class description looks quite good. I think this sentence can be simplified down to "Charset provider are deployed on the application module path ...".

Fixed in 08e8161

Thanks for the approval @AlanBateman . Could you also review the CSR when you get the chance.

justin-curtis-lu · 2024-09-27T18:24:15Z

Thanks for the reviews.
/integrate

openjdk · 2024-09-27T18:26:10Z

Going to push as commit 082125d.
Since your change was applied there have been 127 commits pushed to the master branch:

a7bfced: 8337679: Memset warning in src/hotspot/share/adlc/adlArena.cpp
f554c3f: 8341059: Change Entrust TLS distrust date to November 12, 2024
65200a9: 8340490: Shenandoah: Optimize ShenandoahPacer
824a297: 8341057: Add 2 SSL.com TLS roots
5aae3d4: 8341096: ProblemList compiler/cha/TypeProfileFinalMethod.java in Xcomp mode
68c4f36: 8340024: In ClassReader, extract a constant for the superclass supertype_index
12de4fb: 8340826: Should not send unload notification for scratch classes
25e8929: 8340620: Fix -Wzero-as-null-pointer-constant warnings for CompressedOops
6587909: 8341015: OopStorage location decoder crashes accessing non-initalized OopStorage
9003e2c: 8341027: Crash in java/runtime/Unsafe/InternalErrorTest when running with -XX:-UseCompressedClassPointers
... and 117 more: https://git.openjdk.org/jdk/compare/bc36ace72c1189dcd6d0c05d40d8c568acd89b01...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-27T18:26:15Z

@justin-curtis-lu Pushed as commit 082125d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

justin-curtis-lu added 2 commits September 18, 2024 16:52

init

42fd1b6

newline in module-info.java

a9cb9db

openjdk bot changed the title ~~JDK-8340404: Clarify CharsetProvider deployment on application module path~~ 8340404: Clarify CharsetProvider deployment on application module path Sep 19, 2024

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Sep 19, 2024

openjdk bot added the nio nio-dev@openjdk.org label Sep 19, 2024

AlanBateman reviewed Sep 19, 2024

View reviewed changes

justin-curtis-lu added 2 commits September 19, 2024 09:34

reflect review comments

790abd0

merge master, resolve conflicts

6aff4f3

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 19, 2024

resolve conflicts; list deployment as module first

1362692

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Sep 19, 2024

naotoj reviewed Sep 19, 2024

View reviewed changes

drop obsolete 'extensions' sentence

b7b5044

AlanBateman reviewed Sep 20, 2024

View reviewed changes

justin-curtis-lu added 2 commits September 20, 2024 09:37

review: 'available' -> 'deployed'

c4056ba

review: spec correction - system class loader used, not context

d90f6ca

justin-curtis-lu changed the title ~~8340404: Clarify CharsetProvider deployment on application module path~~ 8340404: CharsetProvider specification updates Sep 21, 2024

AlanBateman reviewed Sep 22, 2024

View reviewed changes

review: simplify wording

08e8161

AlanBateman approved these changes Sep 23, 2024

View reviewed changes

naotoj approved these changes Sep 23, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 27, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 27, 2024

openjdk bot closed this Sep 27, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8340404: CharsetProvider specification updates #21076

8340404: CharsetProvider specification updates #21076

justin-curtis-lu commented Sep 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 19, 2024

openjdk bot commented Sep 19, 2024 •

edited

Loading

openjdk bot commented Sep 19, 2024

mlbridge bot commented Sep 19, 2024 •

edited

Loading

AlanBateman Sep 19, 2024

justin-curtis-lu Sep 19, 2024

AlanBateman Sep 20, 2024

justin-curtis-lu Sep 20, 2024

AlanBateman Sep 21, 2024

justin-curtis-lu Sep 21, 2024

AlanBateman Sep 19, 2024

AlanBateman Sep 19, 2024

AlanBateman Sep 19, 2024

openjdk bot commented Sep 19, 2024

naotoj Sep 19, 2024

AlanBateman Sep 20, 2024

justin-curtis-lu Sep 20, 2024

AlanBateman Sep 22, 2024

justin-curtis-lu Sep 23, 2024

justin-curtis-lu Sep 23, 2024

justin-curtis-lu commented Sep 27, 2024

openjdk bot commented Sep 27, 2024

openjdk bot commented Sep 27, 2024

8340404: CharsetProvider specification updates #21076

8340404: CharsetProvider specification updates #21076

Conversation

justin-curtis-lu commented Sep 19, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 19, 2024

openjdk bot commented Sep 19, 2024 • edited Loading

openjdk bot commented Sep 19, 2024

mlbridge bot commented Sep 19, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-curtis-lu commented Sep 27, 2024

openjdk bot commented Sep 27, 2024

openjdk bot commented Sep 27, 2024

justin-curtis-lu commented Sep 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 19, 2024 •

edited

Loading

mlbridge bot commented Sep 19, 2024 •

edited

Loading