8373825: [lworld] ForwardingFileManager always closes its delegate leading to ClosedFileSystemException in TestOriginatingElements

[david-beaumont]

Allow users of ForwardingFileManager to specify if the delegate file manager should also be closed.

This fixes a use-after-close bug in TestOriginatingElements, where the system file manager and wrapping memory file manager have different, nested, lifetimes resulting in an early call to close the system file manager when the wrapper is closed, causing subsequent legitimate use of the system file manager to fail.

This wasn't noticed before now since the system file manager was using the singleton JRT file-system in which "close" does nothing. Now it's using a properly scoped instance of the JRT file-system which can be closed.

This fix could also probable go straight into mainline as a clone since it won't break anything there.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Error

 ⚠️ 8373825 is used in problem lists: [test/langtools/ProblemList.txt, test/langtools/ProblemList-enable-preview.txt]

Issue

• JDK-8373825: [lworld] ForwardingFileManager always closes its delegate leading to ClosedFileSystemException in TestOriginatingElements (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1810/head:pull/1810
$ git checkout pull/1810

Update a local copy of the PR:
$ git checkout pull/1810
$ git pull https://git.openjdk.org/valhalla.git pull/1810/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1810

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1810.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 03: Full - Incremental (1b29ac47)
• 02: Full - Incremental (d41fac6b)
• 01: Full - Incremental (d49af5d7)
• 00: Full (5b780501)

[david-beaumont]

I could leave this alone, but it seemed nicer to actually acknowledge that the file managers are closeable.

[david-beaumont]

Only called in 2 places, so migrating callers with an extra parameter is easy.

[david-beaumont]

This is the vast majority of users, they don't care about the delegated instance and want it closed.

[lahodaj]

I think the standard way to change behavior for Forwarning... classes is to simply override the method in the subclass, and implement whatever logic needed - delegating or not. In this case, subclasses can override the close method to do nothing. While I agree the close is a little bit special, I am not convinced it is special enough to warrant a field and a new constructor.

For MemoryFileManager, a new constructor parameter is fine I think - that's a specialized class and not an API.

[david-beaumont]

Fair enough, that was my other thought, but since it's analogous to various I/O file/stream forwarding APIs which do explicitly call out whether the delegate should be closed, I thought I'd push on that route first.

[david-beaumont]

Actually the more I think about it, the more I'm convinced it's a better API if the forwarding class knows whether to close or not. The reasoning is this:

• There is only one correct answer to "should be delegate be closed or not", there is no safe default.
• The existing code assumes a behaviour which could lead to hard-to-debug bugs.
• By adding a new constructor, the question of lifetime ownership becomes visible to people using the class.

While it's not a forcing function (the other constructor with original behaviour must still exist), there's more chance users encountering the class for the first time will think about the issue, compared to having to just remember that lifetime ownership is a thing and what the forwarding class does.