Fixes transfers to File when append? is true
#56
Conversation
Move javac options to top-level in project.clj Indentation changes
Add unit tests covering issue
| (when (pos? n) | ||
| (recur (+ idx n))))) | ||
| ;; .position() is 0 for normal mode, and the current size of the file in append mode | ||
| (loop [pos (.position fc)] |
There was a problem hiding this comment.
Tests are passing without this modification.
Without a deep knowledge of byte-streams, I'm wondering whether we are acting at the right place here.
If I understand correctly, the position defines where on the ReadableByteChannel has to start, not on the ẀriteableByteChannel. [1]
But I might be totally wrong here.
There was a problem hiding this comment.
I’ll double-check, but the new tests were failing on my Mac. The original issue claimed it didn’t happen on Linux…is that your platform?
There was a problem hiding this comment.
It’s the reverse about the position. It defines where in the FC to start writing, regardless of the FC’s current position. The RBC’s position advances normally as data is read.
If the transferFrom fn used the FC position, append mode would work by default.
That being said, I’m concerned that it may be platform-specific, because that means CI may miss a regression.
There was a problem hiding this comment.
I reverted it, and my tests failed again.
There was a problem hiding this comment.
I’ll double-check, but the new tests were failing on my Mac. The original issue claimed it didn’t happen on Linux…is that your platform?
Yes it is, but I got caught be your comment:
I believe append-mode for File transfers should have been broken for all platforms, except when "appending" to empty files.
So I confirm, it's working on Linux with and without your change.
There was a problem hiding this comment.
That’s crazy. I wonder if we discovered a Linux jdk bug. Which Java version did you test on?
the Java code quickly hands off to native code, nothing I saw would explain this.
There was a problem hiding this comment.
❯ java -version
openjdk version "17.0.1" 2021-10-19
OpenJDK Runtime Environment Temurin-17.0.1+12 (build 17.0.1+12)
OpenJDK 64-Bit Server VM Temurin-17.0.1+12 (build 17.0.1+12, mixed mode, sharing)
There was a problem hiding this comment.
Maybe the getChannel is behaving differently according to the platform.
[1] : https://github.com/clj-commons/byte-streams/blob/master/src/byte_streams.clj#L589
There was a problem hiding this comment.
Yeah, that's pretty likely, but I couldn't find anything online about it. I looked at the underlying FileChannelImpl class, and it dispatches a bunch to native-based FileDescriptors and FileDispatcher classes.
I can try more variations in the tests. Maybe fill it with data first, then close and reopen, to ensure we're not starting with an empty file.
Deprecate old test files Remove empty byte-streams-reload-test
Fixes #23...I think.
I have no idea why it would have worked on Linux, but not a Mac. I believe append-mode for File transfers should have been broken for all platforms, except when "appending" to empty files.
The current fix calls .position() on the FileChannel to know where to start. Normally a fresh FC will be at 0, except in append mode, when .position() will always return the current size of the file.
This doesn't handle multiple writers correctly, since it never calls .position() again, but there's a lot more work to do to make byte-streams compatible with that assumption, so we're not tackling that here.