greglook / clj-cbor

Native Clojure CBOR codec implementation.
The Unlicense
70 stars 7 forks source link

Directly coerce to DataOutputStream without using output-stream #14

Closed axel-angel closed 5 years ago

axel-angel commented 5 years ago

This is problematic because io/output-stream in clojure adds yet an other buffer in the middle which is uncessary since all instances in the code are OutputStream already and DataOutputStream already accepts it, secondly it creates corruption while using different streams like GZIPOutputStream for unknown reasons the BufferedOutputStream introduced by io/output-stream is preventing the last flush thus corrupting the output.

To reproduce the bug try this:

> (with-open [fd (->> "myfile.cbor.gz" (clojure.java.io/output-stream) (java.util.zip.GZIPOutputStream.))] (clj-cbor.core/encode clj-cbor.core/default-codec fd (into [] (range 1e6))))    
4868653                                                                                                                      
> (with-open [fd (->> "myfile.cbor.gz" (clojure.java.io/input-stream) (java.util.zip.GZIPInputStream.))] (doall (clj-cbor.core/decode clj-cbor.core/default-codec fd)))                    

Execution error (ExceptionInfo) at clj-cbor.error/codec-exception! (error.clj:8).                                            
Input data ended while parsing a CBOR value.

The code above works if we do:

(with-open [fd (->> "myfile.cbor.gz" (clojure.java.io/output-stream) (java.util.zip.GZIPOutputStream.) (java.io.DataOutputStream.))] (clj-cbor.core/encode clj-cbor.core/default-codec fd
(into [] (range 1e6))))

As you can see we directly provide ourself the DataOutputStream thus in the code above we don't call io/output-stream as explained.

codecov-io commented 5 years ago

Codecov Report

Merging #14 into develop will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop    #14   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           13     13           
  Lines          805    804    -1     
======================================
- Hits           805    804    -1
Impacted Files Coverage Δ
src/clj_cbor/core.clj 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 67e09a5...3181151. Read the comment docs.

axel-angel commented 5 years ago

@greglook Oh! that's very good point, I totally forgot the double wrap indeed, a bit stupid. So I did adapt the function and only call it where the stream comes externally. I hope I got it right this time. Thanks!