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
JDK-8260694: (fc) Clarify FileChannel.transferFrom to better describe "no bytes available" case #2329
Conversation
👋 Welcome back mli! A progress list of the required criteria for merging this PR into |
@Hamlin-Li 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
|
/label add nit |
/label add nio |
@AlanBateman The label
|
@AlanBateman |
Webrevs
|
interesting, why it keeps failing? I just modified the javadoc in the patch. is this an environment issue? |
…escribe "no bytes available" case
c9ed084
to
91233c7
Compare
* @apiNote | ||
* This method only returns the number of bytes that were actually | ||
* transferred, it will not return -1. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transferTo method is file -> target channel and I think it would be confusing to mention -1 here.
We could add an apiNote to point out that the method returns zero if the given position is greater than the file's current size, and also zero if the target channel is non-blocking and has no bytes free in its output buffer, but I think the javadoc is already clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will remove this @APinote addition
* This method only returns the number of bytes that were actually | ||
* transferred, it will not return -1, e.g. in case of EOF of the | ||
* {@code src} parameter. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, I think it may be better if we just add a sentence to the end of the second paragraph of the method description. The second paragraph already covers the case that fewer bytes are transferred. I think it can be something simple like "No bytes are transferred if the source channel has reached end-of-stream". I suggest "end-of-stream" rather than "EOF" because that is the phrase used in the RBC javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Alan, Thanks for reviewing.
I will add the sentence you suggested at the end of second paragraph, will use "end-of-stream".
Still think it's better to add an apiNote, although it's a little redundant, but it helps emphasize it, I saw several cases of misunderstanding of transferFrom. How do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the other verbiage I don't think that an apiNote
is actually necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Brian,
At the beginning, I don't think this patch is necessary, because I think if I read really carefully, then I can avoid the misuse.
But the truth is there are several misuse cases, not just in my company, several are in opensource projects. So, I think it's worth to emphasize it even if it's a little redudant.
How do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transferXXX methods return the number of bytes that were transferred to or from the file so there should be no expectation that -1 is returned. My preference would be to just clarify the transferFrom javadoc to make it clear that zero is returned when the source has reached end-of-stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I have removed the apiNote and only refine the javadoc as Brian suggested.
Would you mind to take another look? Thanks
…escribe "no bytes available" case
@@ -642,7 +642,8 @@ public abstract long transferTo(long position, long count, | |||
* number of bytes will be transferred if the source channel has fewer than | |||
* {@code count} bytes remaining, or if the source channel is non-blocking | |||
* and has fewer than {@code count} bytes immediately available in its | |||
* input buffer. | |||
* input buffer. No bytes are transferred if the source channel has reached | |||
* end-of-stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about * end-of-stream in which case zero will be returned.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will modify as you suggested.
@AlanBateman @bplb Hi Alan, Brian, do you know how can I avoid the failures in pre-submit tests? I don't think my patch will introduce the failures, any clue or suggestion? |
…escribe "no bytes available" case
@@ -642,7 +642,8 @@ public abstract long transferTo(long position, long count, | |||
* number of bytes will be transferred if the source channel has fewer than | |||
* {@code count} bytes remaining, or if the source channel is non-blocking | |||
* and has fewer than {@code count} bytes immediately available in its | |||
* input buffer. | |||
* input buffer. No bytes are transferred if the source channel has reached | |||
* the end-of-stream in which case zero will be returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for dropping the apiNote that mentions -1 and return value.
The update is mostly okay, I might re-word it slightly to "No bytes are transferred, and zero is returned, if the source has reached end-of-stream".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alan, just modified as you suggested, I think it's better.
/csr |
@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
Hi Alan, CSR is created at https://bugs.openjdk.java.net/browse/JDK-8261135. Thanks |
…escribe "no bytes available" case
Thanks, I've tweaked it a bit and reduced the specification section down to the changes that we have agreed in the PR. You should be able to hit "Finalize".
Sorry, I can't help on this as I haven't built on 32-bit for several years. There are a few people in OpenJDK that are keeping the 32-bit builds working. |
Thanks Alan, just finalized it.
|
Hi @Hamlin-Li, to move this forward, could you re-finalize the CSR and change the title of this PR in line with the CSR and issue? |
Hi Julia, |
@Hamlin-Li 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:
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 415 new commits pushed to the
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 |
Seems it's ready, am I good to integrate the change? |
Yes, good to go, enter "/integrate" or use pr integrate. |
Thanks Alan. |
/integrate |
@Hamlin-Li Since your change was applied there have been 423 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit a180a38. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
We'd better put more detailed messages in FileChannel.transferFrom/transferTo that it will not return -1, as we met several cases where developers of framework/program assume that it will return -1 on some error conditions, e.g. EOF of src Channel of transferFrom.
please check more details in JDK-8260486, and discussion in https://mail.openjdk.java.net/pipermail/nio-dev/2021-January/008094.html
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2329/head:pull/2329
$ git checkout pull/2329