Skip to content
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.

Sign up for GitHub

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

Test isopen on response_stream #470

Merged
merged 5 commits into from
Sep 29, 2021
Merged

Test isopen on response_stream #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f676525
Test `isopen` on `response_stream`
omus Sep 29, 2021
d6aeacc
fixup! Test `isopen` on `response_stream`
omus Sep 29, 2021
6457586
Appease formatter
omus Sep 29, 2021
8aa2679
Apply suggestions from code review
omus Sep 29, 2021
29886a7
Update test/issues.jl
omus Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions test/issues.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,37 @@ try

try
S3.put_object(BUCKET_NAME, file_name)

# The tests below validate the current behavior of how streams are handled.
# Note: Avoid using `eof` for these tests can hang when using an unclosed `Base.BufferStream`
omus marked this conversation as resolved.
Show resolved Hide resolved

stream = S3.get_object(BUCKET_NAME, file_name, Dict("return_stream" => true))
println("test #466") # So we know if this is the reason for tests hanging.
@test eof(stream) # This will hang if #466 not fixed and using HTTP.jl v0.9.15+
println("#466 fixed")
if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
omus marked this conversation as resolved.
Show resolved Hide resolved
@test !isopen(stream)
else
@test isopen(stream)
end

stream = Base.BufferStream()
S3.get_object(BUCKET_NAME, file_name, Dict("response_stream" => stream))
if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do different backends have different behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They really shouldn't. I think this all comes the fact that HTTPBackend returns a Base.BufferStream by default while DownloadsBackend returns an IOBuffer by default. For this PR I'm just recording the current behaviour and avoiding changing it because as we know closing/not closing a file can be a breaking behaviour

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to address these inconsistencies with #384

Copy link
Contributor

@nickrobinson251 nickrobinson251 Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, i think the changes here would benefit from a comment explaining that they are testing the current behaviour and also linking to the issue you intend to open about possibly changing this behaviour (at first i thought we were testing we had "buggy" behaviour, which seemed an odd thing to test, but now i understand)

The changes themselves all LGTM, i was just struggling to follow what it is that these tests are documenting

@test !isopen(stream)
else
# See: https://github.com/JuliaCloud/AWS.jl/issues/471
@test_broken isopen(stream)
omus marked this conversation as resolved.
Show resolved Hide resolved
omus marked this conversation as resolved.
Show resolved Hide resolved
end

stream = Base.BufferStream()
S3.get_object(
BUCKET_NAME,
file_name,
Dict("response_stream" => stream, "return_stream" => true),
)
if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
@test !isopen(stream)
else
@test isopen(stream)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's expected to be test_broken above, but to work here -- why is that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason for the difference in behaviour. Calling read_body may or may not close the IO:

AWS.jl/src/utilities/downloads_backend.jl

Lines 35 to 39 in 1722aa5

read_body(x::IOBuffer) = take!(x)
function read_body(x::IO)
close(x)
return read(x)
end

Whether this function is called is dependent on a couple of factors:

AWS.jl/src/utilities/downloads_backend.jl

Lines 70 to 74 in 1722aa5

body_arg = if request.request_method == "HEAD" || request.return_stream
() -> NamedTuple()
else
() -> (; body=read_body(request.response_stream))
end

end
finally
S3.delete_object(BUCKET_NAME, file_name)
end
Expand Down
42 changes: 42 additions & 0 deletions test/minio.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,48 @@ try
@test sort(getindex.(objs_prefix["Contents"], "Key")) == ["empty", "myobject"]
@test objs_prefix["CommonPrefixes"]["Prefix"] == "foo/"

# 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"

try
S3.put_object("anewbucket", file_name)

# Note: Using `eof` for these tests can hang when using an unclosed `Base.BufferStream`

stream = S3.get_object("anewbucket", file_name, Dict("return_stream" => true))
if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
@test !isopen(stream)
else
@test isopen(stream)
end

stream = Base.BufferStream()
S3.get_object("anewbucket", file_name, Dict("response_stream" => stream))
if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
@test !isopen(stream)
else
@test_broken isopen(stream)
end

stream = Base.BufferStream()
S3.get_object(
"anewbucket",
file_name,
Dict("response_stream" => stream, "return_stream" => true),
)
if AWS.DEFAULT_BACKEND[] isa AWS.HTTPBackend
@test !isopen(stream)
else
@test isopen(stream)
end
finally
S3.delete_object("anewbucket", file_name)
end
end

finally
# Delete all objects and the bucket
objs = S3.list_objects_v2("anewbucket")
Expand Down