jdk 8292016: Rework JLI_ReportErrorMessageSys

trafficstars

Second attempt at resolving JDK-8292016 with a less intrusive approach this time

Side note: While it might be preferred to remove this entirely, other areas of the JDK also use the same flawed logic, and removing this would simply push the issue elsewhere. Fixing this might result in a solution which can be used in those areas in the future.

Progress

[ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue

Issue

JDK-8292016: Rework JLI_ReportErrorMessageSys

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9870/head:pull/9870
$ git checkout pull/9870

Update a local copy of the PR:
$ git checkout pull/9870
$ git pull https://git.openjdk.org/jdk pull/9870/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9870

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9870.diff

Aug 14 '22 16:08 TheShermanTanker

:wave: Welcome back jwaters! 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.

Aug 14 '22 16:08 bridgekeeper[bot]

@TheShermanTanker The following label will be automatically applied to this pull request:

core-libs

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

Aug 14 '22 16:08 openjdk[bot]

/label hotspot

Aug 14 '22 16:08 TheShermanTanker

@TheShermanTanker The hotspot label was successfully added.

Aug 14 '22 16:08 openjdk[bot]

Webrevs

Aug 14 '22 17:08 mlbridge[bot]

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Sep 17 '22 07:09 bridgekeeper[bot]

@TheShermanTanker Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

Sep 21 '22 18:09 openjdk-notifier[bot]

@TheShermanTanker Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

Sep 26 '22 15:09 openjdk-notifier[bot]

This seems to be trying to future-proof the reporting code by introducing a duality that is not actually needed at present in some cases. That means the code is untested. Are there planned uses of RUNTIME in the very near future?

The only use of it is to differentiate Windows API errors from the regular errno ones thus far, I doubt this will change any time soon. Honestly speaking I'm not very happy with the changes here, and it's been pretty tricky trying to come up with an elegant solution to this issue, but I'm not sure if the alternative of deleting all the error reporting utilities across the JDK and using what C already has (for instance perror) is acceptable in this case, because pretty much all of them have this same problem (and is also why I refrained from outright removing JLI_ReportErrorMessageSys initially like Thomas suggested). On the flip side, all of them don't seem to be used in too many places, and can be rather easily replaced, so it is a little tempting to remove them and just sidestep the problem instead. Are any of these a part of the public JNI API?

Sep 27 '22 16:09 TheShermanTanker

Are any of these a part of the public JNI API?

No this is all internal-use-only

Sep 28 '22 01:09 dholmes-ora

Ah alright, will try removing all of them and checking whether behaviour changes rise up in that case. Will push if none result from the change

Sep 28 '22 12:09 TheShermanTanker

I skimmed through the latest version. I think most of the changes at the use-sites need to be reverted as they are just too confusing and error prone. Maybe this PR should be changed back to draft until there is some agreement on this topic?

Oct 04 '22 06:10 AlanBateman

JNU_ThrowByNameWithStrerror JNU_ThrowByNamePerror JNU_ThrowIOExceptionWithIOError

The problem with this kind of naming is that the user of the API for which these functions are then called, should not have to know anything about the origins of the actual error code/string. The call-sites really do want to say ThrowXXXWithLastError.

That's a good point for the first 2, but I do feel like it would be helpful to specify the kind of error the utilities that are more specialized (NET_ThrowNewWithLastError and JNU_ThrowIOExceptionWithLastError for instance), since both use very different subsystems on Windows and POSIX, which means they can be segregated accordingly. In my opinion the specific error type could be specified after "Last" instead of replacing it as a compromise. The tricky part comes with the first 2 (JNU_ThrowByNameWithLastError and JNU_ThrowByNameWithMessageAndLastError), which are for general use, but their original implementations fell back to the initial problematic mixing of WIN32 and errno errors, and leaving their names unchanged might just result in more ambiguity at the callsites they're used at. I also don't think I came up with particularly good names for them though, and honestly I'm at a bit of a loss as to what should be done with them at this point, hopefully more reviews can come in with some insight on this end.

Maybe I'm wrong to believe that they can be ignorant of the details but the level of abstraction seems wrong to me.

The issue with that was Thomas's initial concerns that you can't really use the last error like this without at least some knowledge about the origin and nature of the actual error itself (WIN32 completely bypassing errno on Windows, and APIs on POSIX arbitrarily using it sometimes but using a return value instead to signal an error other times). I did try to address that by making it a little more specific with this patch, but it still seems there's a better way to do it than this...

I'll try to address the other reviews in the meantime though, before coming back to this.

Oct 04 '22 12:10 TheShermanTanker

Closing - Will narrow the fix to JLI for now in another changeset

Oct 10 '22 12:10 TheShermanTanker

jdk jdk copied to clipboard

8292016: Rework JLI_ReportErrorMessageSys

Progress

Issue

Reviewing

Webrevs

jdk
jdk copied to clipboard