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.

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

merged 5 commits into from
Sep 29, 2021

Conversation

omus
Copy link
Member

@omus omus commented Sep 29, 2021

Follow up to #467 and #466. The original test for issue #466 used eof which could cause the tests to hang if the code was configured incorrectly. The tests have been revised to use isopen which doesn't have this issue.

I further improved upon the tests by checking the behaviour of using "response_stream" and "return_stream". As it turns out it has revealed a bug with the DownloadsBackend which I won't be fixing at this time (#471)

Finally, I also duplicated this test suite for MinIO as this allows this behaviour to be tested locally and not just on the CI. I can remove this if desired.

test/issues.jl Outdated Show resolved Hide resolved
test/minio.jl Outdated Show resolved Hide resolved
@omus
Copy link
Member Author

omus commented Sep 29, 2021

Note I've tested these changes locally using HTTP 0.9.14 and 0.9.15

@omus omus requested a review from mattBrzezinski September 29, 2021 14:14
@omus
Copy link
Member Author

omus commented Sep 29, 2021

cc: @nickrobinson251

@omus
Copy link
Member Author

omus commented Sep 29, 2021

bors try

bors bot added a commit that referenced this pull request Sep 29, 2021
@bors
Copy link
Contributor

bors bot commented Sep 29, 2021

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

Changing to isopen seems a good idea.

I don't understand the behaviour of different backends -- i know you said there's a bug in Downloads.jl-backend, but from the code itself i don't follow what the expected behaviour is and which bits are broken/buggy. It looks like we expect different behaviour from the different backends, but that is surprising to me, so i think warrants a comment with a link to an issue explaining

test/issues.jl Show resolved Hide resolved

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

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

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:

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:

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

test/issues.jl Show resolved Hide resolved
test/issues.jl Show resolved Hide resolved
test/issues.jl Show resolved Hide resolved
@omus
Copy link
Member Author

omus commented Sep 29, 2021

I'll squash and merge this PR as this doesn't need to be more than a single commit

test/issues.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@omus omus merged commit 146dd67 into master Sep 29, 2021
@omus omus deleted the cv/isopen-response-stream branch September 29, 2021 15:37
omus added a commit that referenced this pull request Sep 29, 2021
omus added a commit that referenced this pull request Sep 29, 2021
omus added a commit that referenced this pull request Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants