8286735: (fc) FileChannel#map should document behavior when using custom file systems

[bplb]

Clarify when the map() methods of java.nio.channels.FileChannel throw an UnsupportedOperationException.

---

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 reviewer)

Issue

• JDK-8286735: (fc) FileChannel#map should document behavior when using custom file systems

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8739

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

Using diff file

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

[bridgekeeper]

👋 Welcome back bpb! 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]

@bplb The following label will be automatically applied to this pull request:

• nio

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

• 00: Full (e76740c1)

[AlanBateman]

I think this feature request will require a lot more discussion on what the right level of optionality is. If a custom file system provider does not support all features (file locking, memory-mapped files, ..) then FileSystemProvider::newFileChannel should throw UOE.

[bplb]

I think this feature request will require a lot more discussion on what the right level of optionality is. If a custom file system provider does not support all features (file locking, memory-mapped files, ..) then FileSystemProvider::newFileChannel should throw UOE.

I wonder whether for the immediate purpose for which this issue was filed the changes to the @throws UnsupportedOperationException clauses should be removed and only the new @implSpec retained?

[AlanBateman]

I think this feature request will require a lot more discussion on what the right level of optionality is. If a custom file system provider does not support all features (file locking, memory-mapped files, ..) then FileSystemProvider::newFileChannel should throw UOE.

I wonder whether for the immediate purpose for which this issue was filed the changes to the @throws UnsupportedOperationException clauses should be removed and only the new @implSpec retained?

JDK-8286735 is a request for finer grain optionality. It cannot be evaluated in isolation because there will be others that will want file locking, or positional read/write to be optional too. The long standing position on this topic is that custom file system providers are not required to support FileChannel (FileSystemProvider::newFileChannel can throw UOE) so I think we have to be cautious about changing this because it would make FileChannel unusable - if you give me a reference to a FileChannel then I couldn't rely on anything if every feature were optional.

The change that adds an implSpec to the new FileChannel.map is okay but it's probably a separate JBS issue.

[bplb]

Perhaps the submitter of this issue (@uschindler) could clarify the intent and modify or split this issue as needed?

[uschindler]

Hi,
sorry if this issue may have caused more friction than intended originally.

Actually this is indeed two issues intermixed, that's my fault. The basic issue after merging Panama into main branch is the following and that was what I stumbled on from the beginning: Filesystems developed for JDK 8 to JDK 18 may return their own implementation of FileChannel. We do this in Lucene during our testing to catch bugs - thats all fine! We subclass FileChannel and implement the abstract methods, often just delegating (e.g. we emulate some windows behaviour in our testing on top of Linux - think of - just as example - differences regarding the issue that you cannot delete files while still open or similar). For that we just create a custom Filesystem that wraps all native FileChannel (and other APIs) to add some "bad behaviour" (like we all know form windows).

Thats all fine and works great - util the new method added in JDK 19 (although it is @Preview feature - I know about that): If you create a wrapper filesystem and just wrap, the whole thing breaks with JDK 19. You won't of course not implement the new method added for Panama. When you the have code calling the new code to get a mmapped MemorySegment from a FileChannel, it thows UnsupportedOperationException (because the code was compiled for JDK 17 where the new method doid not exist). Of course that's expected with JDK 19, but it is not documented like that! That was the issue we have seen. Very simple.

This was the reason why I opened the issue: In JDK 19, if you create your own FileSystem that returns a FileChannel for a file , this impl does not know that there's a new method in JDK 19. In contrast, the Javadocs are wrong in that case stating "If an unsupported map mode is specified." If you want to split the issue and just fix the issue for the new method added in JDK 19: Let's just do it! The current Javadocs are wrong, because the default implementation also throws UOE (because mmapping a file as MemorySegment) is not supported by a FileChannel by default (unless supported and the new JDK-19-style implementation exists).

Because I know that often custom Filesystems do not support memory mapping, I was suggesting to add that javadoc also to the legacy MappedByteBuffer code. If that's not wanted remove that. It was just a suggestion to change the spec if you already do it for 19. That's all.

Does this sound OK to you?

[uschindler]

If you don't agree that the spec should be changed for the old API spec, let's remove that statement and only fix the new method's documentation.

[bplb]

If you don't agree that the spec should be changed for the old API spec, let's remove that statement and only fix the new method's documentation.

If we were to add the proposed @implSpec, then I don't think the @throws UnsupportedOperationException verbiage for map(MapMode, long, long, MemorySession) would need to be tweaked as the UOE is always thrown. If that would be acceptable, then perhaps the present issue could be revised and another separate issue created to address the optionality concerns that Alan raised, if that is something which it is desirable to investigate separately.

[uschindler]

I havo no strong preference where the information about this implemnation detail is written down.

I agree to change the original issue and add a new one about optionality.

[bplb]

Withdrawing this request in favor of #9095.