JDK-8276892: Provide a way to emulate exceptional situations in FileManager when using JavadocTester

[jonathan-gibbons]

Please review a moderately simple addition to the JavadocTester world, to be able to modify the behavior of a file manager to throw exceptions as needed.

The bulk of the new functionality is in a new class, TestJavaFileManagerBuilder, which uses the builder pattern to allow a client to configure and build a file manager that can provide file objects for which the behavior can be overridden for one or more methods. The expected use case is to throw an exception instead of calling the underlying delegate method.

JavadocTester is modified to allow a file manager to be provided when invoking javadoc. This requires some minor changes to the outermost javadoc tool classes, Main and Start. Rather than add more static methods to Main, instance methods are now provided to make it easier to configure the values that will be passed to Start. In Start, it was previously assumed that either the default file manager was being used or that all paths would be configured directly in the file manager. The latter part of that assumption is reduced so that path-setting options (e.g. --source-path, --class-path etc) can be passed to the provided file manager. (Previously, they were silently ignored.) It is an error to pass path-like options as javadoc options to a file manager that does not support them. However, since none of the changes are visible to any public API, this should not be an issue.

A new test is provided, with a few simple test cases. One is for direct use of the new file manager mechanism (without using javadoc). The other two illustrate how the feature can be used inside a JavadocTester call of javadoc. Given that the feature is a relatively simple combination of predicates and proxies, it's not clear that we need a significant body of test cases.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8276892: Provide a way to emulate exceptional situations in FileManager when using JavadocTester

Reviewers

• Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 9130979

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6404/head:pull/6404
$ git checkout pull/6404

Update a local copy of the PR:
$ git checkout pull/6404
$ git pull https://git.openjdk.java.net/jdk pull/6404/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6404

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6404.diff

[bridgekeeper]

👋 Welcome back jjg! 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.

[openjdk]

@jonathan-gibbons The following label will be automatically applied to this pull request:

• javadoc

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.

[mlbridge]

Webrevs

• 05: Full - Incremental (f988f464)
• 04: Full - Incremental (91309791)
• 03: Full - Incremental (69c7d350)
• 02: Full - Incremental (d008ab3a)
• 01: Full - Incremental (5ae2d75c)
• 00: Full (1bdc0185)

[pavelrappo]

Initial comments below.

[jonathan-gibbons]

Here is additional note/suggestion ...

The basic API for the builder is very, well, basic, using Predicate<JFO> and Method objects.
In time, we might want to add some convenience fields or methods to the builder to reduce call-site clutter from custom lambdas.

Early drafts of the code provided overloads for the handle method, but writing the tests showed it was hard to anticipate what might be required ... so providing constants or methods to use with the basic handle method may be a good way to go.

[pavelrappo]

After having looked at this change, I think I might have fallen prey to the famous "XY problem". Instead of asking on how to emulate error conditions in a black-box fashion first, I asked to provide white-box support for those through JavadocTester.

Just to make sure we're on the same page and aren't trying to solve a more complex problem than is required. Is there an easy way to make these methods throw: javax.tools.JavaFileManager#getFileForInput and javax.tools.FileObject#getCharContent?

[jonathan-gibbons]

After having looked at this change, I think I might have fallen prey to the famous "XY problem". Instead of asking on how to emulate error conditions in a black-box fashion first, I asked to provide white-box support for those through JavadocTester.

I didn't see the question this way, and I thought the JBS issue was well stated.

Setting aside JavadocTester for a moment, there were two ways to solve the general problem of triggering exceptions to emulate weird conditions.

1. Build back doors into the main system JavaFileManager ... i..e. JavacFileManager (note Javac), such that the code can be triggered by backdoor access, either reflective, or breaking abstraction or system properties.
2. Provide a new file manager to do the job.

I prefer not to do the first ... I prefer not to build those backdoors into the main file manager that is routinely used everywhere. So that means to do the second.

Providing a new file manager to do the job means either using javax.tools.DocumentationTool directly, or else updating JavadocTester. The latter seems more desirable because of all the extra code in JavadocTester.javadoc(...) surrounding the actual tool invocation. And it comes at the cost of tweaking the tool invocation. It would be possible to have JavadocTester invoke Start directly without modification, but that would have required modifying all existing tests, because of the need to use com.sun.tools.javac.util.Context. Stepping away from that means modifying either Main or Start to accept a file manager directly, and of the two, it seemed more future-proof to adapt Main into a builder-style API, without otherwise changing any existing code paths.

The one surprise/disappointment was the "sort-of technical debt" I discovered in Start ... that file-manager options in the "command-line" args were ignored if a non-standard file manager was used. That seemed like a worthwhile issue to address here.

In terms of providing a custom file manager, the question is, "which file objects" and "which methods". I went through a series of iterations before deciding on using Predicate<JavaFileObject> to filter the file objects. For a while, the API used regular expressions applied to the name of the file object, but that runs into issues with platform-specific file separators. Yeah, we could use the URI, but equally, it's nicer to have access to the file object itself, for filtering directly on properties of the file object, including the possibility of .equals(). As for "which methods", yeah, you suggested the two methods used to read objects, but what if we want to write tests where the exceptions are thrown when writing the object.

That all being said, I accept that the API as presented at this point may seem to be a sledgehammer, and it may be convenient to provide a coat of paint. I like the general underlying architecture of the solution, but maybe we can come up with a method on the builder along the following lines:

static StandardJavaFileManager throwExceptionWhenReading(StandardJavaFileManager fm, JavaFileObject jfo)

We can bike-shed on the name, and maybe there's a 3rd argument Function<JavaFileObject, Throwable> to create the exception to supply the exception that will be thrown. And, there's clearly a family of possible overloads, such as replacing the JavaFileObject parameter with a Predicate<JavaFileObject> to make it easier to specify the behavior for more than one file object, if that should be useful, or Predicate<String> to specify a predicate on the file name, or last component of the filename.

To summarize, I don't think the change is as scary as it might appear at first, although it may be somewhat lacking in ease-of-use for common cases. While the code of the builder may seem complicated, with its use of reflection and proxies, the entire class, the file is only 240 lines, including the legal header block and documentation comments.

Just to make sure we're on the same page and aren't trying to solve a more complex problem than is required. Is there an easy way to make these methods throw: javax.tools.JavaFileManager#getFileForInput and javax.tools.FileObject#getCharContent?

TL;DR: Yes. But I didn't know about the getFileForInput use case. I'll look to make that possible.

[jonathan-gibbons]

TL;DR: Yes. But I didn't know about the getFileForInput use case. I'll look to make that possible.

There is an interesting difference between trapping javax.tools.JavaFileManager#getFileForInput and javax.tools.FileObject#getCharContent.

For getCharContent we are filtering on the receiver of the method call. For getFileForInput we would want to trap on the result.

[jonathan-gibbons]

(Sorry for the accidental closed/re-open)

[pavelrappo]

For getFileForInput we would want to trap on the

That sentence ends abruptly.

[jonathan-gibbons]

For getFileForInput we would want to trap on the

That sentence ends abruptly.

I had meant to delete the comment, because I wasn't ready to finish it.; that must be how/why I accidentally closed the PR. For now, I have completed the sentence, but generally, you can ignore the comment. I'll follow up when I have more concrete ideas.

[pavelrappo]

After having looked at this change, I think I might have fallen prey to the famous "XY problem". Instead of asking on how to emulate error conditions in a black-box fashion first, I asked to provide white-box support for those through JavadocTester.

I didn't see the question this way, and I thought the JBS issue was well stated.

Setting aside JavadocTester for a moment, there were two ways to solve the general problem of triggering exceptions to emulate weird conditions.

1. Build back doors into the main system JavaFileManager ... i..e. JavacFileManager (note Javac), such that the code can be triggered by backdoor access, either reflective, or breaking abstraction or system properties.
2. Provide a new file manager to do the job.

[...]

I should've asked an even more general question before creating JDK-8276892: How can I trigger an error from specific operations on a FileManager and a FileObject returned from that FileManager?

A simulation through a custom implementation of these types that this PR proposes would be one answer. However, such a simulation is the white-box approach, and as far as I understand, JDK contributors traditionally prefer the black-box approach.

In the black-box approach, one seeks ways to emulate the environment that would yield the desired effect. For example, to trigger IOException on an attempt to delete a file, I would ensure that the path does not exist or that the JVM has insufficient file-system permissions to access that path. Here's another example. To achieve socket blocking on write, I would fill it in with as much data as possible (SO_SNDBUF) while simultaneously ensuring that no one reads from it.

If you say that a black-box approach is impossible or impractical here, I'll be fine with the white-box approach along the lines of what this PR suggests.

[jonathan-gibbons]

After having looked at this change, I think I might have fallen prey to the famous "XY problem". Instead of asking on how to emulate error conditions in a black-box fashion first, I asked to provide white-box support for those through JavadocTester.

I didn't see the question this way, and I thought the JBS issue was well stated.
Setting aside JavadocTester for a moment, there were two ways to solve the general problem of triggering exceptions to emulate weird conditions.

1. Build back doors into the main system JavaFileManager ... i..e. JavacFileManager (note Javac), such that the code can be triggered by backdoor access, either reflective, or breaking abstraction or system properties.
2. Provide a new file manager to do the job.

[...]

I should've asked an even more general question before creating JDK-8276892: How can I trigger an error from specific operations on a FileManager and a FileObject returned from that FileManager?

[...]

If you say that a black-box approach is impossible or impractical here, I'll be fine with the white-box approach along the lines of what this PR suggests.

I don't know of a way to reliably trigger the exceptions with the current primary file manager ... i.e. JavacFileManager (note the c). You might be able to force exceptions by playing with file system permissions, but that may be problematic to do in a platform-independent way.

Here's a different way of looking at it, as an instance of a more general problem. JavaFileManager is effectively a service-provider interface, to provide an interface between "files" and javac. At the time it was written, the only access to files was via java.io.File and there was interest in providing access to "virtual files" such as in-memory buffers, as may be found in an IDE. (JavaFileManager appeared in JDK 6, and so predates the NIO Path and FileSystem APIs, which appeared later, in JDK 7; if NIO had been available in JDK 6, we might not have written JavaFileManager). So, as a service-provider interface, methods were declared to throw exceptions that might be expected to occur on as yet unknown implementations. Thus, there is no guarantee that any specific implementation will need or be able to throw any of the declared exceptions.

For the situation that motivated this issue, the desire is to test/cover the catch blocks, so it does not seem so important to me how the exceptions are generated ... whether by tricking the raw JavacFileManager into throwing exceptions, or by wrapping it and using reflective proxy code, as here. The one thing I take away is to maybe try and improve ease-of-use, perhaps with more convenience or utility methods.

[pavelrappo]

I can work with that to test snippets; thanks. Update the copyright years before integrating.

[openjdk]

@jonathan-gibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8276892: Provide a way to emulate exceptional situations in FileManager when using JavadocTester

Reviewed-by: prappo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 587 new commits pushed to the master branch:

• 22c15dd: 8279189: Inaccurate comment about class VMThread
• 9d99a37: 8277881: Missing SessionID in TLS1.3 resumption in compatibility mode
• 4669bcd: 8279244: test accompaning fix for JDK-8205187 is failing in Windows
• a3b1c6b: Merge
• 04ad668: 8279204: [BACKOUT] JDK-8278413: C2 crash when allocating array of size too large
• 730f670: 8268297: jdk/jfr/api/consumer/streaming/TestLatestEvent.java times out
• 9d5ae2e: 8279076: C2: Bad AD file when matching SqrtF with UseSSE=0
• 04ee921: 8278967: rmiregistry fails to start because SecurityManager is disabled
• 2be3e7e: 8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d
• 9df200f: 8278795: Create test library and tests for langtools snippets
• ... and 577 more: https://git.openjdk.java.net/jdk/compare/bce35ac1d6c4115148468a3240ad459074e0b79e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[jonathan-gibbons]

/integrate

[openjdk]

Going to push as commit d52392c.
Since your change was applied there have been 587 commits pushed to the master branch:

• 22c15dd: 8279189: Inaccurate comment about class VMThread
• 9d99a37: 8277881: Missing SessionID in TLS1.3 resumption in compatibility mode
• 4669bcd: 8279244: test accompaning fix for JDK-8205187 is failing in Windows
• a3b1c6b: Merge
• 04ad668: 8279204: [BACKOUT] JDK-8278413: C2 crash when allocating array of size too large
• 730f670: 8268297: jdk/jfr/api/consumer/streaming/TestLatestEvent.java times out
• 9d5ae2e: 8279076: C2: Bad AD file when matching SqrtF with UseSSE=0
• 04ee921: 8278967: rmiregistry fails to start because SecurityManager is disabled
• 2be3e7e: 8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d
• 9df200f: 8278795: Create test library and tests for langtools snippets
• ... and 577 more: https://git.openjdk.java.net/jdk/compare/bce35ac1d6c4115148468a3240ad459074e0b79e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jonathan-gibbons Pushed as commit d52392c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.