RFR: 8318971 : Better Error Handling for Jar Tool Processing of "@File"

[Jaikiran Pai]

On Wed, 13 Dec 2023 18:16:56 GMT, Weibing Xiao <duke at openjdk.org> wrote:

> Better Error Handling for Jar Tool Processing of "@File" <br>
> 
> This is a new PR for this PR since the original developer left the team. See all of the review history at https://github.com/openjdk/jdk/pull/16423.
> 
> Thank you.

Hello Weibing, I had another look at this today. The implementation change looks OK. But I think the JBS issue summary and the JBS issue description will need to be updated.

The real issue here isn't specific to the use of `@file` by the `jar` tool. Instead the issue is about an internal detail of the `jar` tool implementation. When the `jar` tool is launched either to create (`-c`) or to update (`-u`) a jar file, it gets passed a list of files to include/update the jar file with. One way to pass those options is literally as part of the command. The other way is to include those files path in a separate file and pass that file to the `jar` tool as the `@that-other-file`. The issue here affects both those variants.

The internal implementation of the `jar` tool in `sun/tools/jar/Main.java`, uses a flag called `ok` to keep track of whether there was any error during the `jar` tool action processing. This `ok` flagged is marked by various methods that get invoked during the `jar` processing. 

When `-c` or `-u` options are passed to the `jar` tool, the `sun/tools/jar/Main.java` calls an internal method `expand()`. This method is responsible for finalizing the files that need to be included in the jar that gets created or updated. This expand method is also responsible for marking that `ok` flag if there are any errors (like any file that was specified to be included/updated in the jar, is missing).  Once the `expand()` method returns, the internal implementation detail of the `jar` tool will go ahead and create a temporary file. Content will be written out to this temporary files. The more entries to create in the final jar being created/updated, the more time and more disk space this temporary file will consume. Finally, when writing out to the temporary file is done, the `ok` flag is checked to see if it was flagged as failed. If `ok` says no errors were noticed, then the temporary file that was created will be moved as the final jar that had to be created/updated. If `o
 k` said that there were failures, then the temporary file is deleted and the final jar to be created won't get created (rightly so) nor will the final jar get updated, in case of `-u` option (rightly so).

The issue here is that if the `expand()` method noticed that there was an error with the provided file paths (like 1 out of the 5 files specified is missing), then it marks the `ok` flag to indicate that, but the current implementation of the `jar` tool (before the change in this PR) just doesn't check the flag and goes on to do the expensive work of creating a temporary file (which will have the rest of the 4 files) and that temporary file will then finally get discarded at the end. This entire process of creating the temporary file can be avoided by checking the `ok` flag after the `expand()` method returns. That is what the proposal in the PR does and I think it looks good.

Given that, I think the JBS issue summary and the description doesn't match what we are doing here. Would you be able to update those accordingly? If you want me to do that, do let me know and you can then update this PR title.

As for the new test method introduced in this PR, in the existing `InputFilesTest`, it took me a while to understand what exactly this was doing. One part of it is due to a bit misleading method names - for example, `println()` method which going by that name, I thought was printing a new line (I didn't understand why) but turns out that method actually prints out the output that has been captured in a `ByteArrayOutputStream` by a  previous invocation of the `jar` tool from within the test method. That `println()` method has been that way even before the changes in this PR, so I think that is OK (unless you won't mind updating the name to something better?).

This existing `InputFilesTest` runs various tests for the `jar` tool by passing it file paths to be included/updated in the jar. It currently doesn't have any coverage for `@file` usages, so this new test method is adding that. However, this new test method isn't exercising the case where one or more file paths specified in the passed `@file` are missing. So it isn't exactly exercising the issue that is being fixed. I think the test you added is OK as a positive test (where all passed file paths are present), but I think we would also need one for missing files and then assert in that test that the final jar isn't created or updated and the `jar` tool returns a non-zero exit code. Please also add some comments to the newly introduced test methods to make it easier to understand what they are testing.

While at it, could you also please check if the `@file` usage has test coverage from some other existing test class? I am bit surprised that this `InputFilesTest` doesn't have any, so maybe it's tested elsewhere?

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

PR Comment: https://git.openjdk.org/jdk/pull/17088#issuecomment-1862849864