clj-commons / byte-streams

A Rosetta stone for JVM byte representations
417 stars 33 forks source link

{:append? true} is ignored on Mac, but not Linux #23

Closed amalloy closed 2 years ago

amalloy commented 8 years ago

The transfer function accepts an options map, and one option that's accepted when the output is a file is :append?, which defaults to true. That option is supposed to be used at https://github.com/ztellman/byte-streams/blob/a7706c1/src/byte_streams.clj#L571-L573 and indeed it seems to work when run on a Linux machine. However, when the same code is run on a macbook, this option seems to do nothing: setting it to true, or false, or not specifying it at all, all result in the output file being clobbered and overwritten with the new contents.

Below is some sample clojure code you can run to reproduce this issue, expressed as a clojure.test deftest; after loading this namespace, you can simply run (append-works) to check for correct behavior. On Mac OS, one test fails; on Linux, they all pass.

Note that this uses ./test.txt as a scratch file, so if you have something important in this location there is some risk to running the tests. I've included a bail-out clause at the beginning to avoid that, but just in case it's probably best not to rely on that, and instead make sure that no such file exists before running the test.

(ns amalloy.test-byte-streams
  (:require [byte-streams :as bs]
            [clojure.java.io :as io]
            [clojure.test :refer [deftest is]])
  (:import (java.io FileOutputStream)
           (java.nio ByteBuffer)))

(def text (.getBytes "abcd"))
(def file (io/file "test.txt"))

(defn contents []
  (slurp file))

(deftest append-works
  (when (.exists file) (throw (RuntimeException. "Refusing to delete existing test.txt file")))
  (bs/transfer text file)
  (is (= "abcd" (contents)) "transferring to deleted file failed")
  (bs/transfer text file {:append? false})
  (is (= "abcd" (contents)) "overwriting with :append? false failed")

  (bs/transfer text file {:append? true})
  (is (= "abcdabcd" (contents)) "appending same contents with :append? true failed")

  (bs/transfer text file {:append? false})
  (is (= "abcd" (contents)) "overwriting with :append? false failed")

  (with-open [out-chan (.getChannel (FileOutputStream. file true))]
    (.write out-chan (ByteBuffer/wrap text)))
  (is (= "abcdabcd" (contents)) "appending by hand with java.nio failed")

  (.delete file))
ztellman commented 8 years ago

Just to be clear, is the "appending by hand" test also failing?

amalloy commented 8 years ago

No, appending by hand works on both systems. It's included to make it clear that the JVM classes work fine in both environments, but somehow byte-streams is not passing the right arguments (or calling the wrong methods, or doing whatever wrong).

ztellman commented 8 years ago

Okay, glad we can correctly assign blame, then. I'll take a look.

imrekoszo commented 8 years ago

This also seems to be an issue on Windows. Traced an issue we were having to this page and ran the tests included - "appending same contents with :append? true failed" fails.

Windows 7, latest java 8.

imrekoszo commented 2 years ago

🎉 thank you!

KingMob commented 2 years ago

@imrekoszo FYI, I have no idea why it would have succeeded on Linux, but not Mac/Windows. The code I changed should (theoretically) have been broken on all platforms.

imrekoszo commented 2 years ago

should (theoretically) have been broken on all platforms.

Yeah I skimmed the discussion in the PR, interesting for sure. Nevertheless, I appreciate the fix!