RFR: 8347712: IllegalStateException on multithreaded ZipFile access with non-UTF8 charset [v7]

[Jaikiran Pai]

On Wed, 30 Apr 2025 13:12:37 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> Jaikiran Pai 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 21 additional commits since the last revision:
>> 
>>  - Eirik's review about code comments
>>  - fix comment typo
>>  - merge latest from master branch
>>  - 8355975: introduce a test for 8355975
>>  - merge latest from master branch
>>  - merge latest from master branch
>>  - merge latest from master branch
>>  - merge latest from master branch
>>  - merge latest from master branch
>>  - improve code comment for ZipFile.zipCoder
>>  - ... and 11 more: https://git.openjdk.org/jdk/compare/9d9004bf...9a29b960
>
> test/jdk/java/util/zip/ZipFile/ZipFileCharsetTest.java line 68:
> 
>> 66:         // ISO-8859-15 is not a standard charset in Java. We skip this test
>> 67:         // when it is unavailable
>> 68:         assumeTrue(Charset.availableCharsets().containsKey(ISO_8859_15_NAME),
> 
> I would suggest throwing SkippedException otherwise junit throws org.opentest4j.TestAbortedException  If I understand correctly

Hello Lance, I was under the impression that the jtreg framework would mark an aborted junit test as skipped. Now that you mentioned this, I checked locally and in its current form, jtreg reports this test as:


STARTED    ZipFileCharsetTest::testCachedZipFileSource 'testCachedZipFileSource()'
org.opentest4j.TestAbortedException: Assumption failed: skipping test since ISO-8859-15 charset isn't available
ABORTED    ZipFileCharsetTest::testCachedZipFileSource 'testCachedZipFileSource()' [37ms]

[ JUnit Containers: found 4, started 4, succeeded 4, failed 0, aborted 0, skipped 0]
[ JUnit Tests: found 1, started 1, succeeded 0, failed 0, aborted 1, skipped 0]




So it's being classified by jtreg as aborted instead of skipped. 

I then took your suggestion to throw `jtreg.SkippedException`:


+import jtreg.SkippedException;
-import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
+ * @library /test/lib
  * @run junit ZipFileCharsetTest
  */
 public class ZipFileCharsetTest {
 
         // ISO-8859-15 is not a standard charset in Java. We skip this test
         // when it is unavailable
-        assumeTrue(Charset.availableCharsets().containsKey(ISO_8859_15_NAME),
-                "skipping test since " + ISO_8859_15_NAME + " charset isn't available");
+        if (!Charset.availableCharsets().containsKey(ISO_8859_15_NAME)) {
+            throw new SkippedException("skipping test since " + ISO_8859_15_NAME
+                    + " charset isn't available");
+        }



That however makes jtreg mark this test as failed instead of skipped:


STARTED    ZipFileCharsetTest::testCachedZipFileSource 'testCachedZipFileSource()'
jtreg.SkippedException: skipping test since ISO-8859-15 charset isn't available
    at ZipFileCharsetTest.testCachedZipFileSource(ZipFileCharsetTest.java:70)
    at java.base/java.lang.reflect.Method.invoke(Method.java:565)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
FAILED     ZipFileCharsetTest::testCachedZipFileSource 'testCachedZipFileSource()' [28ms]
JavaTest Message: JUnit Platform Failure(s): 1

[ JUnit Containers: found 4, started 4, succeeded 4, failed 0, aborted 0, skipped 0]
[ JUnit Tests: found 1, started 1, succeeded 0, failed 1, aborted 0, skipped 0]


So it looks like we have an issue here with jtreg when it runs a junit test and the test throws `jtreg.SkippedException`. There appears to be an open issue for that (although in context of testng) https://bugs.openjdk.org/browse/CODETOOLS-7902676.

Given this, I can't think of a different way to handle this situation. Would it be OK to continue using `assumeTrue(...)` for now?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068769646