RFR: 8306580: Propagate CDS dumping errors instead of directly exiting the VM [v5]

[Ioi Lam]

On Thu, 13 Jun 2024 21:29:30 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Currently, when CDS dumping run into an unrecoverable error (e.g., file I/O error, out of memory), it calls MetaspaceShared::unrecoverable_writing_error(), which directly exits the VM.  Some of these errors can be propagated to the caller for a normal exit. 
>> 
>> This change introduces `MetaspaceShared::writing_error()` to report errors without exiting the VM. The function `MetaspaceShared::unrecoverable_writing_error()` now should only be used for errors that require the VM to exit. Verified with tier1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reverted OOME check and Calvin comment

Changes requested by iklam (Reviewer).

src/hotspot/share/cds/metaspaceShared.cpp line 659:

> 657:       log_error(cds)("%s: %s", PENDING_EXCEPTION->klass()->external_name(),
> 658:                      java_lang_String::as_utf8_string(java_lang_Throwable::message(PENDING_EXCEPTION)));
> 659:       MetaspaceShared::writing_error("VM exits due to exception, use -Xlog:cds,exceptions=trace for detail");

As we don't exit immediately, how about:


MetaspaceShared::writing_error("Unexpected exception: use ....")

src/hotspot/share/cds/metaspaceShared.cpp line 809:

> 807:   if (PrintSystemDictionaryAtExit) {
> 808:     SystemDictionary::print();
> 809:   }

This "if" block should be removed. We had to do it long time ago because we used to exit the VM immediately after the archive was dumped. Now we let the execution continue, so the dictionary will eventually be printed in `print_statistics()` in java.cpp

test/hotspot/jtreg/runtime/cds/StaticWritingError.java line 55:

> 53:         if (System.getProperty("os.name").startsWith("Windows")) {
> 54:             // Windows filesystem uses Access Control Lists instead of permissions
> 55:             Path dir = Files.createTempDirectory(directoryName);

`Files.createTempDirectory()` may create a file in a system-wide directory such as `C:\Temp`, which may not be automatically cleaned up after the test finished. See [here](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/nio/file/Files.html#createTempDirectory(java.lang.String,java.nio.file.attribute.FileAttribute...))

It seems that on Windows, you are creating a directory like `C:\Temp\unwritable.temp123456` but run with `-XX:SharedArchiveFile=unwritable\staticWritingError.jsa". The JVM fails because the `unwritable` directory doesn't exist.

If this is true, I think you can simplify the test on all platforms without setting the write permission:


"-XX:SharedArchiveFile=nosuchdir" + File.separator + archiveName

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

PR Review: https://git.openjdk.org/jdk/pull/19370#pullrequestreview-2117143245
PR Review Comment: https://git.openjdk.org/jdk/pull/19370#discussion_r1639021561
PR Review Comment: https://git.openjdk.org/jdk/pull/19370#discussion_r1639025167
PR Review Comment: https://git.openjdk.org/jdk/pull/19370#discussion_r1639027708