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

8261938: ASN1Formatter.annotate should not return in the finally block #2620

Closed
wants to merge 3 commits into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Feb 18, 2021

Hi all,

ASN1Formatter.annotate should be able to throw an IOException according to this comment [1].
But it fails to do so due to the return [2] in the finally block, which would swallow the IOException.

Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3].

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113
[2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120
[3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning


Progress

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

Issue

  • JDK-8261938: ASN1Formatter.annotate should not return in the finally block

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620
$ git checkout pull/2620

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 18, 2021

/issue add JDK-8261938
/test
/label add core-libs
/cc core-libs

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 18, 2021

👋 Welcome back jiefu! 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 openjdk bot added the rfr label Feb 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 18, 2021

@DamonFool This issue is referenced in the PR title - it will now be updated.

@openjdk openjdk bot added the core-libs label Feb 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 18, 2021

@DamonFool
The core-libs label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 18, 2021

@DamonFool The core-libs label was already applied.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 18, 2021

Webrevs

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

An EOFException can occur during the call to annotate() and must return the accumulated contents of the StringBuffer. Otherwise it is discarded.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 18, 2021

Thanks @RogerRiggs for your review.

Just want to make sure:

  1. AFAIK, for a method, it seems impossible to return a value and throw an exception at the same time.
    Did you mean we just need to return a string with the IOException to be catched?

  2. Does it make sense to return a string when an IOException happens?

Thanks.

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Feb 19, 2021

The formattters are a test component used both standalone and in the context of the HexPrinter test utilities.
In typical use, the stream is a wrapped byte array, so there are no exceptions other than EOF.
The choice of DataInputStream was chosen for the convenience of the methods to read different types
and (declared) exceptions are an unwelcome artifact.

Formatters are designed to be nested, where one formatter can call another and the valuable output
is the formatted string that has been accumulated from the beginning of the stream.
If an exception was percolated up and the formatted output discarded it would defeat the purpose.

If an exception was thrown, it would still return useful information about the stream to that point.
The documentation could be improved to be clear on that point.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 19, 2021

The formattters are a test component used both standalone and in the context of the HexPrinter test utilities.
In typical use, the stream is a wrapped byte array, so there are no exceptions other than EOF.
The choice of DataInputStream was chosen for the convenience of the methods to read different types
and (declared) exceptions are an unwelcome artifact.

Formatters are designed to be nested, where one formatter can call another and the valuable output
is the formatted string that has been accumulated from the beginning of the stream.
If an exception was percolated up and the formatted output discarded it would defeat the purpose.

If an exception was thrown, it would still return useful information about the stream to that point.
The documentation could be improved to be clear on that point.

Got it.
Thanks for your clarification.

Updated.
Thanks.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Catching and ignoring the exception has the same behavior as handling it with finally.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 19, 2021

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

8261938: ASN1Formatter.annotate should not return in the finally block

Reviewed-by: rriggs

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

  • 433096a: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
  • efbaede: 8262018: Wrong format in SAP copyright header of OsVersionTest
  • 55463b0: 8261984: Shenandoah: Remove unused ShenandoahPushWorkerQueuesScope class
  • a180a38: 8260694: (fc) Clarify FileChannel.transferFrom to better describe "no bytes available" case
  • 1b0c36b: 8261649: AArch64: Optimize LSE atomics in C++ code
  • 61820b7: 8259984: IGV: Crash when drawing control flow before GCM
  • 7e2c909: 8260485: Simplify and unify handler vectors in Posix signal code
  • c99eeb0: 8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
  • 5caf686: 8261644: NMT: Simplifications and cleanups
  • ed93bc9: 8196301: java/awt/print/PrinterJob/Margins.java times out
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/ea5bf45c6f1bdcc7d671d9c7258ed98280637331...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 Feb 19, 2021
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Feb 19, 2021

Thanks @RogerRiggs .
/integrate

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

@openjdk openjdk bot commented Feb 19, 2021

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

  • 977a21a: 8261225: TieredStopAtLevel should have no effect if TieredCompilation is disabled
  • c53acc2: 8261542: X86 slice and unslice intrinsics for 256-bit byte/short vectors
  • 8b4fd77: 8262042: ProblemList javax/xml/jaxp/unittest/common/prettyprint/PrettyPrintTest.java on Windows
  • 7ffa148: 8247918: Clarify Reader.skip behavior for end of stream
  • 8a1c712: 8261728: SimpleDateFormat should link to DateTimeFormatter
  • 851b2e3: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance
  • c4f17a3: 8257925: enable more support for nested inline tags
  • 433096a: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
  • efbaede: 8262018: Wrong format in SAP copyright header of OsVersionTest
  • 55463b0: 8261984: Shenandoah: Remove unused ShenandoahPushWorkerQueuesScope class
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/ea5bf45c6f1bdcc7d671d9c7258ed98280637331...master

Your commit was automatically rebased without conflicts.

Pushed as commit b10376b.

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

@DamonFool DamonFool deleted the DamonFool:JDK-8261938 branch Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants