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

[Jie Fu-2]

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

-------------

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

Changes: https://git.openjdk.java.net/jdk/pull/2620/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261938
  Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2620.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620

PR: https://git.openjdk.java.net/jdk/pull/2620

[Jie Fu-2]

> 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

Jie Fu has updated the pull request incrementally with one additional commit since the last revision:

  Update the copyright year

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2620/files
  - new: https://git.openjdk.java.net/jdk/pull/2620/files/c0af12b1..b537e060

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2620.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620

PR: https://git.openjdk.java.net/jdk/pull/2620

[Roger Riggs-2]

On Thu, 18 Feb 2021 02:46:53 GMT, Jie Fu <[hidden email]> wrote:

>> 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
>
> Jie Fu has updated the pull request incrementally with one additional commit since the last revision:
>
>   Update the copyright year

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

-------------

Changes requested by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2620

[Jie Fu-2]

On Thu, 18 Feb 2021 14:53:17 GMT, Roger Riggs <[hidden email]> wrote:

>> Jie Fu has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Update the copyright year
>
> An EOFException can occur during the call to annotate() and must return the accumulated contents of the StringBuffer.  Otherwise it is discarded.

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2620

[Roger Riggs-2]

On Thu, 18 Feb 2021 23:41:08 GMT, Jie Fu <[hidden email]> wrote:

>> An EOFException can occur during the call to annotate() and must return the accumulated contents of the StringBuffer.  Otherwise it is discarded.
>
> 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.

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2620

[Jie Fu-2]

> 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

Jie Fu has updated the pull request incrementally with one additional commit since the last revision:

  Catch exception and return the accumulated string

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2620/files
  - new: https://git.openjdk.java.net/jdk/pull/2620/files/b537e060..a1f29f33

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=01-02

  Stats: 16 lines in 1 file changed: 13 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2620.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620

PR: https://git.openjdk.java.net/jdk/pull/2620

[Jie Fu-2]

On Fri, 19 Feb 2021 01:56:19 GMT, Roger Riggs <[hidden email]> wrote:

> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2620

[Roger Riggs-2]

On Fri, 19 Feb 2021 03:36:00 GMT, Jie Fu <[hidden email]> wrote:

>> 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
>
> Jie Fu has updated the pull request incrementally with one additional commit since the last revision:
>
>   Catch exception and return the accumulated string

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

-------------

Marked as reviewed by rriggs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2620

[Jie Fu-2]

On Thu, 18 Feb 2021 02:28:01 GMT, Jie Fu <[hidden email]> wrote:

> 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

This pull request has now been integrated.

Changeset: b10376ba
Author:    Jie Fu <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/b10376ba
Stats:     15 lines in 1 file changed: 10 ins; 1 del; 4 mod

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

Reviewed-by: rriggs

-------------

PR: https://git.openjdk.java.net/jdk/pull/2620

[Jie Fu-2]

On Fri, 19 Feb 2021 15:15:05 GMT, Roger Riggs <[hidden email]> wrote:

>> Jie Fu has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Catch exception and return the accumulated string
>
> Catching and ignoring the exception has the same behavior as handling it with finally.

Thanks @RogerRiggs .

-------------

PR: https://git.openjdk.java.net/jdk/pull/2620