Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264148: Update spec for exceptions retrofitted for exception chaining #3182

Closed
wants to merge 5 commits into from

Conversation

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Mar 24, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264148: Update spec for exceptions retrofitted for exception chaining

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3182/head:pull/3182
$ git checkout pull/3182

Update a local copy of the PR:
$ git checkout pull/3182
$ git pull https://git.openjdk.java.net/jdk pull/3182/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3182

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3182.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 24, 2021

👋 Welcome back darcy! 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
Copy link

@openjdk openjdk bot commented Mar 24, 2021

@jddarcy The following labels will be automatically applied to this pull request:

  • beans
  • compiler
  • core-libs
  • i18n
  • security
  • serviceability

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

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Mar 25, 2021

The removal of the obsolescent "As of release 1.4, this exception has been retrofitted..." is good. Changing the calls from the other exception-getting methods to getCause() is also good. I'm less sure of the utility of deprecating these older methods. The deprecation will issue warning messages; is there any benefit to the calling code to migrating from the older methods to getCause()? If they're exactly equivalent, then I think the benefits are small, compared to the cost of dealing with warnings. Thus for most of these cases I think that not deprecating the older methods is reasonable, and perhaps the explanation should be converted to an @apiNote.

(The considerations for the JDK itself are different, though, which is why I support changing the call sites.)

One special case is the public field in WriteAbortedException. This is really bad and something ought to be done about this, including deprecation, and maybe more. This implies that the exception is mutable, right? Hrrmph. Isn't there a general rule that once the cause has been set (either via a constructor or via initCause) the exception is immutable? Maybe the field should be deprecated, and getCause() should return the cause from the superclass. That's a behavior change of course, and I don't know how to assess the compatibility impact. But the current situation just seems wrong.

@jddarcy jddarcy marked this pull request as ready for review Mar 29, 2021
@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Mar 29, 2021

Some discussion of the proposed changes here. While the history with respect to the introduction of the chained exception mechanism may have been relevant in times past, those comments have little utility today. Per guidance from Dr. Deprecator, I've removed deprecation of the redundant methods and only kept deprecation of the mutable field in WriteAbortedException.

The legacy wrapped exception accessing methods in ClassNotFoundException, ExceptionInInitializerError, and UndeclaredThrowableException are equivalent to getCause. The legacy method in PrivilegedActionException returns the same value, but narrowed to Exception rather than Throwable.

Once the rest of the change is agree to, I'll update copyrights and do a CSR for the deprecation and other spec changes.

@openjdk openjdk bot added the rfr label Mar 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Webrevs

@jmehrens
Copy link

@jmehrens jmehrens commented Mar 30, 2021

One special case is the public field in WriteAbortedException. This is really bad and something ought to be done about this, including deprecation, and maybe more. This implies that the exception is mutable, right? Hrrmph. Isn't there a general rule that once the cause has been set (either via a constructor or via initCause) the exception is immutable? Maybe the field should be deprecated, and getCause() should return the cause from the superclass. That's a behavior change of course, and I don't know how to assess the compatibility impact. But the current situation just seems wrong.

Agreed. WriteAbortedException should get similar treatment as the others like it:

The only twist is that we would have to keep the public field as deprecated.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

I agree that the public field in WriteAbortedException could be remediated.
But it is also mostly harmless.

if (ite.getTargetException() instanceof RuntimeException) {
throw (RuntimeException)ite.getTargetException();
if (ite.getCause() instanceof RuntimeException) {
throw (RuntimeException)ite.getCause();

This comment has been minimized.

@RogerRiggs

RogerRiggs Mar 30, 2021
Contributor

This might be a place to use the new instanceof pattern form:
if (ite.getCause() instanceof RuntimeException rex) throw rex.getCause();

@@ -288,7 +288,7 @@ public static Object newStringConstructor(String type, String param)
try {
return c.newInstance(param);
} catch (InvocationTargetException e) {
Throwable t = e.getTargetException();
Throwable t = e.getCause();
if (t instanceof Exception) {
throw (Exception) t;

This comment has been minimized.

@RogerRiggs

RogerRiggs Mar 30, 2021
Contributor

Ditto:
if (t instanceof Exception ex) throw ex

@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

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

8264148: Update spec for exceptions retrofitted for exception chaining

Reviewed-by: rriggs, smarks

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

  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • daeca3f: 8262958: (fs) UnixUserDefinedFileAttributeView cleanup
  • af02883: 8264191: Javadoc search is broken in Internet Explorer
  • 6e74c3a: 8264193: Remove TRAPS parameters for modules and defaultmethods
  • ee5e00b: 8264279: Shenandoah: Missing handshake after JDK-8263427
  • ac604a1: 8264374: Shenandoah: Remove leftover parallel reference processing argument
  • f3726a8: 8264020: Optimize double negation elimination
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/128c0c97a592839146dd0cf631f8c0f76bec9fd5...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 openjdk bot added the ready label Mar 30, 2021
@@ -87,6 +81,7 @@ public String getMessage() {
* which may be null.
* @since 1.4
*/
@Override
public Throwable getCause() {
return detail;

This comment has been minimized.

@jmehrens

jmehrens Mar 30, 2021

Use SuppressWarnings??

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 30, 2021

Mailing list message from Joe Darcy on beans-dev:

On 3/30/2021 6:43 AM, jmehrens wrote:

On Wed, 24 Mar 2021 23:17:46 GMT, Joe Darcy <darcy at openjdk.org> wrote:

8264148: Update spec for exceptions retrofitted for exception chaining
src/java.base/share/classes/java/io/WriteAbortedException.java line 86:

84: @Override
85: public Throwable getCause() {
86: return detail;
Use SuppressWarnings??

There is no warning to suppress since the detail field is defined in the
same class.

-Joe

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 30, 2021

Mailing list message from Joe Darcy on beans-dev:

On 3/30/2021 6:29 AM, Roger Riggs wrote:

On Wed, 24 Mar 2021 23:17:46 GMT, Joe Darcy <darcy at openjdk.org> wrote:

8264148: Update spec for exceptions retrofitted for exception chaining
I agree that the public field in WriteAbortedException could be remediated.
But it is also mostly harmless.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMObjectFactory.java line 62:

60: catch (java.lang.reflect.InvocationTargetException ite) {
61: if (ite.getCause() instanceof RuntimeException) {
62: throw (RuntimeException)ite.getCause();
This might be a place to use the new instanceof pattern form:
`if (ite.getCause() instanceof RuntimeException rex)
throw rex.getCause();
`

src/jdk.jconsole/share/classes/sun/tools/jconsole/inspector/Utils.java line 293:

291: Throwable t = e.getCause();
292: if (t instanceof Exception) {
293: throw (Exception) t;
Ditto:
` if (t instanceof Exception ex) throw ex`

I think the use of the new instanceof form would be better left for a
follow-up refactoring.

Thanks,

-Joe

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Mar 30, 2021

/integrate

@openjdk openjdk bot closed this Mar 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@jddarcy Since your change was applied there have been 23 commits pushed to the master branch:

  • 353807c: 8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win)
  • 2bd80f9: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation
  • b08d638: 8262503: Support records in Dynalink
  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • daeca3f: 8262958: (fs) UnixUserDefinedFileAttributeView cleanup
  • af02883: 8264191: Javadoc search is broken in Internet Explorer
  • 6e74c3a: 8264193: Remove TRAPS parameters for modules and defaultmethods
  • ... and 13 more: https://git.openjdk.java.net/jdk/compare/128c0c97a592839146dd0cf631f8c0f76bec9fd5...master

Your commit was automatically rebased without conflicts.

Pushed as commit 815248a.

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

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Joe,
The Serviceability part looks good.
Thanks,
Serguei

@jddarcy jddarcy deleted the jddarcy:8264148 branch Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment