8265989: System property for the native character encoding name #3777
Conversation
|
/csr |
|
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
|
@naotoj this pull request will not be integrated until the CSR request JDK-8266075 for issue JDK-8265989 has been approved. |
Webrevs
|
| @@ -699,6 +699,9 @@ public static native void arraycopy(Object src, int srcPos, | |||
| * <td>User's home directory</td></tr> | |||
| * <tr><th scope="row">{@systemProperty user.dir}</th> | |||
| * <td>User's current working directory</td></tr> | |||
| * <tr><th scope="row">{@systemProperty native.encoding}</th> | |||
| * <td>Character encoding name derived from the host environment and/or | |||
| * the user's settings. Setting this system property has no effect.</td></tr> | |||
There was a problem hiding this comment.
May be "This property is read-only" instead of "Setting this system property has no effect" to not confuse with "user's settings"?
There was a problem hiding this comment.
I suspect that if setProperty("native.encoding", "foo") succeeds, then it will return "foo". I also believe that a later invocation of getProperty("native.encoding") will also return "foo". If that's the case, then I don't think that the "read-only" alternative phrasing is correct. To me, the alternative suggests that an error will be return if there is an attempt to set it or that the potential new value will be ignored. The "no effect" phrasing avoids this problem. I also suspect that the "no effect" phrasing was selected to align with the apiNote in lines 719-721.
There was a problem hiding this comment.
Thanks, Joe and Iris. I agree with Iris and that's the reason I chose the description. System properties are inherently mutable. There are some "protected" ones, by that I mean a private copy is made just after initialization for VM use, which is not affected by later setProperty() calls. But I don't think this new property should be treated as such.
There was a problem hiding this comment.
The value of native.encoding should be a static property; that is, read once at startup and later modification
does not change the behavior of the Java runtime.
Whereas the value of native.encoding is derived from the value of native variables and those native
values do not change while Java is running, I think the behavior of the Java runtime should stay the same.
Unless it is static, the Java runtime will need to read the property every time it is needed and behavior can
change from call to call and actions in one thread can affect other threads.
Is there a use case where the application needs to change the encoding for every use in the Java runtime
independently of the native values.
Such an application should be explicit about its charset requirements and use APIs to select them explicitly.
There was a problem hiding this comment.
Ok, understandable, and having a statement to clearly say 'no effect' would be indeed helpful. While working on methods with a charset parameter some time ago, I remember reading some user discussions about unable to programmatically change the default encoding, a confusion mostly as the user attempted to set encoding, e.g. System.setProperty("file.encoding", encoding), it seems that "the property is set (meaning no error), but it did not have the desired effect". Then there was also a mention in some tutorial where file.encoding and sun.jnu.encoding were recognized as read-only. The problem or confusion was that it appeared there’s such an API and the operation was successful, but it really didn’t have the effect.
| @@ -80,6 +80,7 @@ | |||
| {"user.dir"}, | |||
| {"java.runtime.version"}, | |||
| {"java.runtime.name"}, | |||
| {"native.encoding"}, | |||
There was a problem hiding this comment.
Does this test differentiate between read-only and modifiable properties? "native.encoding" looks like it's explicitly overridable. Or do we need a test to verify it's not overridable?
| var nativeEncoding = ((raw.propDefault(Raw._file_encoding_NDX) == null) | ||
| ? raw.propDefault(Raw._sun_jnu_encoding_NDX) | ||
| : raw.propDefault(Raw._file_encoding_NDX)); | ||
| put(props, "native.encoding", nativeEncoding); |
There was a problem hiding this comment.
Shouldn't native.encoding be biased toward sun.jnu.encoding rather than file.encoding? Or maybe you'll change it when preparing the changes for JEP 400?
There was a problem hiding this comment.
native.encoding preserves the encoding that current Charset.defaultCharset() is returning, which is based on file.encoding. So I believe the current implementation is correct. If it is biased toward sun.jnu.encoding, it would be problematic especially on Windows where that represents system locale.
There was a problem hiding this comment.
Okay for now but the value of file.encoding will change to "UTF-8" so will be different to native.encoding.
There was a problem hiding this comment.
Yes. That will be covered by JEP400.
|
Naive question: any reason as to why we're not providing a new static API method in Charset to return the platform encoder? This initially will return same thing as Charset.defaultEncoder - but as JEP 400 is delivered the two will diverge. Any reason as to why we don't want to expose the platform encoder in the API? |
Ok, I see this is addressed in the CSR: The problem with this approach is that I think clients that need the platform encoder will have to stash it somewhere in a static field (to prevent reading property and parsing value over and over). It might also be harder for javadoc (I'm thinking of some of the Panama API) to express a dependency on it. |
Agree an API would be better. We've looked at it a few times but have concerns that it would be confusing to most developers, esp. if it were another static method on Charset. System and Runtime have been examined too. The proposal here doesn't preclude adding an API in the future, it's mostly a question on whether it is needed and where would it be defined. |
|
Unless there is some use case other than for migration, I would not introduce a static method as Alan mentioned. |
Thanks. Added. |
|
@naotoj 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: 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 169 new commits pushed to the
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 |
|
/integrate |
|
@naotoj Since your change was applied there have been 180 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 4e96b31. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
After some internal discussion, we thought it was good to expose the native environment's default character encoding, which Charset.defaultCharset() is currently based on. This way applications will have a better migration path after the JEP 400 is implemented, in which Charset.defaultCharset() will return UTF-8, but the value of this new system property will remain intact. A CSR has been filed with more detailed information.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3777/head:pull/3777$ git checkout pull/3777Update a local copy of the PR:
$ git checkout pull/3777$ git pull https://git.openjdk.java.net/jdk pull/3777/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3777View PR using the GUI difftool:
$ git pr show -t 3777Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3777.diff