Use BufferStream as a sacrificial I/O (#475)

* Use BufferStream as sacrificial I/O

* Set project version to 1.63.1

* Use `expected` in tests

Co-authored-by: mattBrzezinski <[email protected]>

* Avoid intermediate read

Co-authored-by: Lyndon White <[email protected]>

Author: omus

Project.toml

@@ -1,7 +1,7 @@
 name = "AWS"
 uuid = "fbe9abb3-538b-5e4e-ba9e-bc94f4f92ebc"
 license = "MIT"
-version = "1.63.0"
+version = "1.63.1"
 
 [deps]
 Base64 = "2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"

src/utilities/request.jl

@@ -158,7 +158,7 @@ function _http_request(http_backend::HTTPBackend, request::Request, response_str
         # stream. Effectively, this works as if we're not using streaming I/O at all but we
         # will keep using the `response_stream` kwarg to ensure we aren't relying on the
         # response's body being populated.
-        buffer = IOBuffer()
+        buffer = Base.BufferStream()
 
         r = try
             @mock HTTP.request(
@@ -172,25 +172,13 @@ function _http_request(http_backend::HTTPBackend, request::Request, response_str
                 http_options...,
             )
         finally
-            # Read directly from the buffer's data as the I/O stream is closed by HTTP.jl.
-            # We need to be careful to always write the buffer data to the `response_stream`
-            # when an exception occurs as this data contains details about the AWS error.
-            data = if isopen(buffer)
-                take!(buffer)
-            else
-                # When closed the `IOBuffer` size will be set to 0. In order to retrieve the
-                # raw data we need to determine the size from the first '\0' character.
-                first_null = findfirst(==(0x00), buffer.data)
-                last_index = if first_null !== nothing
-                    first_null - 1
-                else
-                    lastindex(buffer.data)
-                end
-
-                buffer.data[firstindex(buffer.data):last_index]
-            end
+            # We're unable to read from the `Base.BufferStream` until it has been closed.
+            # HTTP.jl will close passed in `response_stream` keyword. This ensures that it
+            # is always closed (e.g. HTTP.jl 0.9.15)
+            close(buffer)
 
-            write(response_stream, data)
+            # Transfer the contents of the `BufferStream` into `response_stream` variable.
+            write(response_stream, buffer)
         end
 
         return @mock Response(r, response_stream)

test/issues.jl

@@ -124,6 +124,37 @@ try
         end
     end
 
+    @testset "issue 474" begin
+        body = "foo\0bar"
+        expected = Vector{UInt8}(body)
+        file_name = "null.txt"
+
+        try
+            S3.put_object(BUCKET_NAME, file_name, Dict("body" => body))
+
+            raw = S3.get_object(BUCKET_NAME, file_name, Dict("return_raw" => true))
+            @test raw == expected
+
+            stream = S3.get_object(BUCKET_NAME, file_name, Dict("return_stream" => true))
+            if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
+                @test stream isa Base.BufferStream
+                @test !isopen(stream)
+
+                if !isopen(stream)
+                    @test read(stream) == expected
+                end
+            else
+                @test stream isa IOBuffer
+                @test isopen(stream)
+
+                seekstart(stream)
+                @test read(stream) == expected
+            end
+        finally
+            S3.delete_object(BUCKET_NAME, file_name)
+        end
+    end
+
 finally
     S3.delete_bucket(BUCKET_NAME)
 end

test/minio.jl

@@ -65,6 +65,7 @@ try
     # Duplicated testset from "test/issues.jl". Useful for testing outside the CI. Ideally,
     # the tests should be revised such that local testing works without having to duplicate
     # testsets.
+
     @testset "issue 466" begin
         file_name = "hang.txt"
 
@@ -104,6 +105,38 @@ try
         end
     end
 
+    @testset "issue 474" begin
+        body = "foo\0bar"
+        expected = Vector{UInt8}(body)
+        file_name = "null.txt"
+        bucket_name = "anewbucket"
+
+        try
+            S3.put_object(bucket_name, file_name, Dict("body" => body))
+
+            raw = S3.get_object(bucket_name, file_name, Dict("return_raw" => true))
+            @test raw isa Vector{UInt8}
+            @test raw == expected
+
+            stream = S3.get_object(bucket_name, file_name, Dict("return_stream" => true))
+            if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
+                @test stream isa Base.BufferStream
+                @test !isopen(stream)
+
+                if !isopen(stream)
+                    @test read(stream) == expected
+                end
+            else
+                @test stream isa IOBuffer
+                @test isopen(stream)
+                seekstart(stream)
+                @test read(stream) == expected
+            end
+        finally
+            S3.delete_object(bucket_name, file_name)
+        end
+    end
+
 finally
     # Delete all objects and the bucket
     objs = S3.list_objects_v2("anewbucket")