8313815: The exception messages printed by jcmd ManagementAgent.start are corrupted on Japanese Windows #1641

yukikimmura · 2023-08-07T00:48:11Z

I would like to fix this issue
because users can not read the exception message of jcmd ManagementAgent on localized Windows platform and can not understand what happened.
Testing:
all serviceability area tests jdk_svc, and a specific test to verify the fix.

Could anyone review the fix please?

Thanks,
Kimura Yukihiro

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

Issue

JDK-8313815: The exception messages printed by jcmd ManagementAgent.start are corrupted on Japanese Windows (Bug - P4)

Reviewers

Kevin Walls (@kevinjwalls - Committer)
Chris Plummer (@plummercj - Committer)
Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1641/head:pull/1641
$ git checkout pull/1641

Update a local copy of the PR:
$ git checkout pull/1641
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1641/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1641

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1641.diff

Webrev

Link to Webrev Comment

… are corrupted on Japanese Windows

bridgekeeper · 2023-08-07T00:48:52Z

👋 Welcome back yukikimmura! 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.

mlbridge · 2023-08-07T00:54:46Z

Webrevs

plummercj · 2023-08-08T19:03:53Z

/label serviceability

openjdk · 2023-08-08T19:05:30Z

@plummercj
The label serviceability is not a valid label.
These labels are valid:

plummercj · 2023-08-08T19:22:25Z

We have the following in agent.properties:

agent.err.invalid.state            = Invalid agent state: {0}

and it is used by:

 public static final String INVALID_STATE =
    "agent.err.invalid.state";
throw new RuntimeException(getText(INVALID_STATE, "Agent already started"));

Shouldn't "Agent already started" also be text that we should localize in the properties files? If we did, I think this issue would go away. Since the above is the only reference to INVALID_STATE, we could just instead have:

agent.err.invalid.state            = Invalid agent state: Agent already started

dcubed-ojdk · 2023-08-08T19:39:12Z

/label add serviceability

openjdk · 2023-08-08T19:40:47Z

@dcubed-ojdk
The label serviceability is not a valid label.
These labels are valid:

yukikimmura · 2023-08-09T08:53:54Z

Hello Chris,
Thank you for your review.
I'll consider it.

Thanks,
Kimura Yukihiro

plummercj · 2023-08-09T17:02:28Z

If you are going to go with the localization of "Agent already started" approach, it should probably be done in 22 and then backported. If you want to just go with your current fix, that's ok to.

yukikimmura · 2023-08-10T02:44:14Z

Hello Chris,
Thank you for your comments.

I think your suggestion is to set a localized exception message or the English message to the RuntimeException.
I'm sorry if I misunderstood.

I prefer to modify jcmd (HotSpotVirtualMachine.java) for two reasons.
First, ManagementAgent.start can specify jmxremote.host, so the encoding may be different from the host running jcmd.
Second, I think setting a localized message to a RuntimeException is not desirable.
It would be nice to change the message to English on the localized environment, but from JDK18, the localized message is output correctly.

I believe it's not necessary to fix JDK22 as this issue doesn't occur owing to JEP 400.

Thanks,
Kimura Yukihiro

kevinjwalls

OK, looks good to me. 8-)

yukikimmura · 2023-08-16T07:17:32Z

Hello Chris,
Thank you for your review.

I would like to go with the current fix, as I replied to your comment.
I received a comment from you that this fix is OK, but please let me know if you have any concerns.
It would be great if you could approve it.
It is my understanding that a pull request must be approved by at least two reviewer in order to be accepted.

Thanks,
Kimura Yukihiro

yukikimmura · 2023-08-20T07:22:04Z

Hello everyone,
Thank you for your help with this issue.
I tested all serviceability area tests.
And I tested a specific test to verify the fix, which I run the test on:
English Windows
Japanese Windows
RHEL LANG/LC_ALL=
RHEL LANG/LC_ALL=C
RHEL LANG/LC_ALL=en_US.UTF-8
RHEL LNAG/LC_ALL=ja_JP.UTF-8

Is there anything left for me to do to get the fix integrated?
I would like to fix this problem because it is very inconvenient for customers to not be able to read error messages in the Japanese Windows environment.

Thanks,
Kimura Yukihiro

kevinjwalls · 2023-08-21T10:48:41Z

Yes that sounds good. I think you can /integrate and we can /sponsor if that is needed.

yukikimmura · 2023-08-21T12:49:27Z

/integrate

Thank you for your quick reply.
Could you please sponsor the fix?

Thanks,
Kimura Yukihiro

openjdk · 2023-08-21T12:51:14Z

@yukikimmura This pull request has not yet been marked as ready for integration.

kevinjwalls · 2023-08-21T15:25:37Z

/sponsor

openjdk · 2023-08-21T15:27:18Z

@kevinjwalls The change author (@yukikimmura) must issue an integrate command before the integration can be sponsored.

yukikimmura · 2023-08-21T20:01:51Z

I'm very sorry, I typed "integrate" before being marked as ready for integration.
I'll type "integrate" again after jdk17-fix-yes has be set and being marked as ready for integration.

Thanks,
Kimura Yukihiro

kevinjwalls · 2023-08-23T09:03:49Z

Yes of course. On the jbs entry, I think that should have been a jdk17u-fix-request label, so I updated it.

openjdk · 2023-08-25T19:53:51Z

@yukikimmura 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:

8313815: The exception messages printed by jcmd ManagementAgent.start are corrupted on Japanese Windows

Reviewed-by: kevinw, cjplummer, phh

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 29 new commits pushed to the master branch:

fdc4130: 8314426: runtime/os/TestTrimNative.java is failing on slow machines
3c426c7: 8312525: New test runtime/os/TestTrimNative.java#trimNative is failing: did not see the expected RSS reduction
f667b35: 8293114: JVM should trim the native heap
1421540: 8276651: java/lang/ProcessHandle tests fail with "RuntimeException: Input/output error" in java.lang.ProcessHandleImpl$Info.info0
d9985d0: 8314730: GHA: Drop libfreetype6-dev transitional package in favor of libfreetype-dev
e84d9dd: 8313701: GHA: RISC-V should use the official repository for bootstrap
e070c31: 8283724: Incorrect description for jtreg-failure-handler option
40add10: 8299827: Add resolved IP address in connection exception for sockets
e830464: 8314552: Fix javadoc tests to work with jtreg 7
49ffbcf: 8314501: Shenandoah: sun/tools/jhsdb/heapconfig/JMapHeapConfigTest.java fails
... and 19 more: https://git.openjdk.org/jdk17u-dev/compare/ca0f148681114b64ba0b40ea77b89a5e9bc68657...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinjwalls, @plummercj, @phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

yukikimmura · 2023-08-26T12:53:38Z

/integrate

openjdk · 2023-08-26T12:55:44Z

@yukikimmura
Your change (at version 3bd7e1a) is now ready to be sponsored by a Committer.

phohensee · 2023-08-26T14:31:05Z

/sponsor

openjdk · 2023-08-26T14:32:24Z

Going to push as commit 7286bb8.
Since your change was applied there have been 29 commits pushed to the master branch:

fdc4130: 8314426: runtime/os/TestTrimNative.java is failing on slow machines
3c426c7: 8312525: New test runtime/os/TestTrimNative.java#trimNative is failing: did not see the expected RSS reduction
f667b35: 8293114: JVM should trim the native heap
1421540: 8276651: java/lang/ProcessHandle tests fail with "RuntimeException: Input/output error" in java.lang.ProcessHandleImpl$Info.info0
d9985d0: 8314730: GHA: Drop libfreetype6-dev transitional package in favor of libfreetype-dev
e84d9dd: 8313701: GHA: RISC-V should use the official repository for bootstrap
e070c31: 8283724: Incorrect description for jtreg-failure-handler option
40add10: 8299827: Add resolved IP address in connection exception for sockets
e830464: 8314552: Fix javadoc tests to work with jtreg 7
49ffbcf: 8314501: Shenandoah: sun/tools/jhsdb/heapconfig/JMapHeapConfigTest.java fails
... and 19 more: https://git.openjdk.org/jdk17u-dev/compare/ca0f148681114b64ba0b40ea77b89a5e9bc68657...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-26T14:32:33Z

@phohensee @yukikimmura Pushed as commit 7286bb8.

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

yukikimmura · 2023-09-19T00:53:12Z

/backport jdk11u-dev

openjdk · 2023-09-19T00:53:22Z

@yukikimmura To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

yukikimmura · 2023-09-20T00:59:22Z

/backport jdk11u-dev

openjdk · 2023-09-20T01:02:03Z

@yukikimmura the backport was successfully created on the branch yukikimmura-backport-7286bb85 in my personal fork of openjdk/jdk11u-dev. To create a pull request with this backport targeting openjdk/jdk11u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 7286bb85 from the openjdk/jdk17u-dev repository.

The commit being backported was authored by Kimura Yukihiro on 26 Aug 2023 and was reviewed by Kevin Walls, Chris Plummer and Paul Hohensee.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk11u-dev:

$ git fetch https://github.com/openjdk-bots/jdk11u-dev.git yukikimmura-backport-7286bb85:yukikimmura-backport-7286bb85
$ git checkout yukikimmura-backport-7286bb85
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk11u-dev.git yukikimmura-backport-7286bb85

8313815: The exception messages printed by jcmd ManagementAgent.start…

4a249da

… are corrupted on Japanese Windows

openjdk bot added the rfr Pull request is ready for review label Aug 7, 2023

Merge master

0e2813d

kevinjwalls approved these changes Aug 11, 2023

View reviewed changes

yukikimmura added 2 commits August 12, 2023 13:28

Merge master

5a5c43c

Merge branch 'master' into 8313815

3bd7e1a

plummercj approved these changes Aug 16, 2023

View reviewed changes

phohensee approved these changes Aug 25, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 25, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 26, 2023

openjdk bot added the integrated Pull request has been integrated label Aug 26, 2023

openjdk bot closed this Aug 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 26, 2023

8313815: The exception messages printed by jcmd ManagementAgent.start are corrupted on Japanese Windows #1641

8313815: The exception messages printed by jcmd ManagementAgent.start are corrupted on Japanese Windows #1641

Uh oh!

Conversation

yukikimmura commented Aug 7, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 7, 2023

Uh oh!

mlbridge bot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

plummercj commented Aug 8, 2023

Uh oh!

openjdk bot commented Aug 8, 2023

Uh oh!

plummercj commented Aug 8, 2023

Uh oh!

dcubed-ojdk commented Aug 8, 2023

Uh oh!

openjdk bot commented Aug 8, 2023

Uh oh!

yukikimmura commented Aug 9, 2023

Uh oh!

plummercj commented Aug 9, 2023

Uh oh!

yukikimmura commented Aug 10, 2023

Uh oh!

kevinjwalls left a comment

Choose a reason for hiding this comment

Uh oh!

yukikimmura commented Aug 16, 2023

Uh oh!

yukikimmura commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjwalls commented Aug 21, 2023

Uh oh!

yukikimmura commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

kevinjwalls commented Aug 21, 2023

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

yukikimmura commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjwalls commented Aug 23, 2023

Uh oh!

openjdk bot commented Aug 25, 2023

Uh oh!

yukikimmura commented Aug 26, 2023

Uh oh!

openjdk bot commented Aug 26, 2023

Uh oh!

phohensee commented Aug 26, 2023

Uh oh!

openjdk bot commented Aug 26, 2023

Uh oh!

openjdk bot commented Aug 26, 2023

Uh oh!

yukikimmura commented Sep 19, 2023

Uh oh!

openjdk bot commented Sep 19, 2023

Uh oh!

yukikimmura commented Sep 20, 2023

Uh oh!

openjdk bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

yukikimmura commented Aug 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 7, 2023 •

edited

Loading

yukikimmura commented Aug 20, 2023 •

edited

Loading

yukikimmura commented Aug 21, 2023 •

edited

Loading

yukikimmura commented Aug 21, 2023 •

edited

Loading