Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8241619: (fs) Files.newByteChannel(path, Set.of(CREATE_NEW, READ)) does not throw a FileAlreadyExistsException when the file exists #2980

Closed
wants to merge 4 commits into from

Conversation

bplb
Copy link
Member

@bplb bplb commented Mar 12, 2021

Please consider this proposal to add @throws FileAlreadyExistsException or modify an existing such throws clause's description in a number of methods in java.nio.file.Files, java.nio.file.spi.FileSystemProvider, and java.nio.channels.FileChannel. The methods affected are open() in FileChannel and various new*() methods in Files and FileSystemProvider. The Files.newByteChannel() methods already documented this exception so this would bring the other methods in line.

A FileAlreadyExistsException is an optional specific exception, i.e., a subclass of IOException intended to provide a more precise description of the error. The package specification of java.nio.file describes optional specific exceptions but those of the other two packages do not. If these exceptions are to be added to the FileChannel and FileSystemProvider classes, then perhaps a similar paragraph (or a link) should be added to their respective package specifications as well.

The change to java.nio.channels.AsynchronousFileChannel is an incidental correction.


Progress

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

Issue

  • JDK-8241619: (fs) Files.newByteChannel(path, Set.of(CREATE_NEW, READ)) does not throw a FileAlreadyExistsException when the file exists

Reviewers

Download

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

To update a local copy of the PR:
$ git checkout pull/2980
$ git pull https://git.openjdk.java.net/jdk pull/2980/head

…es not throw a FileAlreadyExistsException when the file exists
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 12, 2021

👋 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 openjdk bot added the rfr Pull request is ready for review label Mar 12, 2021
@openjdk
Copy link

openjdk bot commented Mar 12, 2021

@bplb To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing

@bplb
Copy link
Member Author

bplb commented Mar 12, 2021

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Mar 12, 2021
@openjdk
Copy link

openjdk bot commented Mar 12, 2021

@bplb has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@bplb please create a CSR request and add link to it in JDK-8241619. This pull request cannot be integrated until the CSR request is approved.

@AlanBateman
Copy link
Contributor

/label add nio

@openjdk openjdk bot added the nio nio-dev@openjdk.org label Mar 14, 2021
@openjdk
Copy link

openjdk bot commented Mar 14, 2021

@AlanBateman
The nio label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Mar 14, 2021

Webrevs

* if a file of that name already exists and the {@link
* StandardOpenOption#CREATE_NEW CREATE_NEW} option is specified
* and the file is being opened for writing
* <i>(optional specific exception)</i>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be the first usage of optional specific exceptions in the java.nio.channels API docs. It's okay to do this but it will require linking to the java.nio.file package description where this is defined. You'll also need to import the exception or use the fully qualified class file here. Probably best to replace "if" with "If" to be consistent with the other exception messages here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the nio label. I had mentioned the package description already and will update that. There is not much consistency in whether the first letter of the @throws verbiage is upper case.

* java.lang.NullPointerException NullPointerException} to be thrown.
* java.lang.NullPointerException NullPointerException} to be thrown. In some
* cases methods which are specified to throw an {@code IOException} may throw
* a more precise <a href="../file/package-summary.html#optspecex">optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terminology in the reference section is "optional specific exception" so it would be more consistent to continue with "specific" rather than switching to "precise" here. I'm in two minds about adding this to the java.nio.channels package description as it's only applicable to the static open methods defined by FileChannel and AsynchronousFileChannel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed "precise" to "specific" and removed update to java.nio.channels package doc.

@AlanBateman
Copy link
Contributor

Are you planning to link the use of "optional specific exception" in FileChannel to the section in the java.nio.file package description? Also if we are adding optional specific exceptions to FileChannel.open then we'll have to do the same for AsynchronousFileChannel.open.

@bplb
Copy link
Member Author

bplb commented Mar 19, 2021

Added "optional specific exception" link to FileChannel.open() and @throws FileAlreadyExistsException to `AsynchronousFileChannel.open().

@bplb
Copy link
Member Author

bplb commented Mar 19, 2021

The CSR https://bugs.openjdk.java.net/browse/JDK-8263623 is also updated.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 22, 2021
@openjdk
Copy link

openjdk bot commented Mar 22, 2021

@bplb 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:

8241619: (fs) Files.newByteChannel(path, Set.of(CREATE_NEW, READ)) does not throw a FileAlreadyExistsException when the file exists

Reviewed-by: alanb

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 190 new commits pushed to the master branch:

  • f62b100: 8263895: Test nsk/jvmti/GetThreadGroupChildren/getthrdgrpchld001/getthrdgrpchld001.cpp uses incorrect indices
  • f84b52b: 8263897: compiler/c2/aarch64/TestVolatilesSerial.java failed with "java.lang.RuntimeException: Wrong method"
  • f08bf4b: 8263891: Changes for 8076985 missed the fix.
  • b2df513: 8261785: Calling "main" method in anonymous nested class crashes the JVM
  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • 6c2220e: 8263861: Shenandoah: Remove unused member in ShenandoahGCStateResetter
  • 5262d95: 8263855: Use the blessed modifier order in java.management/naming
  • 6f1bcb0: 8263593: Fix multiple typos in hsdis README
  • ... and 180 more: https://git.openjdk.java.net/jdk/compare/e5ce97b12d6bf060419dffb983d0dececdc3ab3a...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 22, 2021
@bplb
Copy link
Member Author

bplb commented Mar 23, 2021

/integrate

@openjdk openjdk bot closed this Mar 23, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 23, 2021
@openjdk
Copy link

openjdk bot commented Mar 23, 2021

@bplb Since your change was applied there have been 214 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 8fa34e4.

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

@bplb bplb deleted the 8241619-Files-newByteChannel branch March 24, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated nio nio-dev@openjdk.org
2 participants