On Tue, 16 Jul 2024 00:37:17 GMT, Bradford Wetmore <wetm...@openjdk.org> wrote:
>> Fernando Guallini has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - added brackets, and refactor a comment and an exception message >> - Regtest java/security/Security/SynchronizedAccess.java is incorrect. > > test/jdk/java/security/Security/SynchronizedAccess.java line 29: > >> 27: * @library /test/lib ../testlibrary >> 28: * @summary Make sure Provider api implementations are synchronized >> properly >> 29: * @run main/othervm SynchronizedAccess > > There are two options here. You can either: > > 1. use `/othervm` and remove the `ProvidersSnapshot`. > 2. Keep the `ProvidersSnapshot` and use `/othervm`. > > The `ProvidersSnapshot` **SHOULD** return the provider list in this agent VM > back to the original state before releasing it to the agentvm for the next > test. That was the point of > [JDK-7054918](https://bugs.openjdk.org/browse/JDK-7054918). If we were > concerned that the test could somehow exit without hitting the finally > (unlikely), then the second option (`/othervm`) would be a safer alternative. Yes, that was my concern, but I would prefer to keep relying on ProvidersSnapshot to revert to the original state. > test/jdk/java/security/Security/SynchronizedAccess.java line 82: > >> 80: } >> 81: Signature.getInstance("sigalg"); >> 82: // Skip the first provider to ensure one is always >> available for getInstance. > > This is forcing a successful `getInstance()`: you might want to check for a > failures too. > > See suggested fix attachment in the overall comments. I originally wanted to avoid a situation where the test could silently fail, such as not having a sigimpl, although it could be for another unexpected reason. Therefore, it was necessary to fail on NSAE. However, since the main intent of the test is to check for unexpected NPEs or bad array indexing, it seems acceptable to ignore the NSAE. I will apply your suggested changes, Thanks! ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1681011704 PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1681012522
