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

Jump to bottom

Fix incomplete get handling via HTTP compat bump and don't write failed retry streams to parent buffer in _http_request #516

Merged
merged 23 commits into from
Nov 25, 2021
Merged

Fix incomplete get handling via HTTP compat bump and don't write failed retry streams to parent buffer in _http_request #516

merged 23 commits into from
Nov 25, 2021

Conversation

IanButterworth
Copy link
Contributor

I believe JuliaWeb/HTTP.jl#781 is the proper fix needed for #515.
That PR introduces an EOFError throw if the http response stream has promised content via Content-length but hits eof.

The issue is rare and only in heavily concurrent operations, and the retry handling in AWS.jl is well set up to handle that

However, there was also an AWS.jl bug that this PR fixes, where each failed retry was being passed to the response_stream.

@IanButterworth IanButterworth mentioned this pull request Nov 17, 2021
Closed
@ericphanson
Copy link
Member

ericphanson commented Nov 17, 2021 •
edited
Loading

heavily concurrent operations

This is totally orthogonal but if you run into JuliaWeb/HTTP.jl#517 style issues (which I certainly did), you might want to try the Downloads.jl based backend to use libcurl instead of HTTP.jl: AWS.DEFAULT_BACKEND[] = AWS.DownloadsBackend() (or on a per-request basis with backend=…).

@IanButterworth
Copy link
Contributor Author

HTTP v0.9.17 is out JuliaRegistries/General#48956

@IanButterworth
Copy link
Contributor Author

Can someone run the CI please 🙏🏻

@mattBrzezinski
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 17, 2021
@bors
Try #516:
b2572a9
@bors
Copy link
Contributor

bors bot commented Nov 17, 2021

try

Build failed:

@IanButterworth IanButterworth changed the title Don't write failed retry streams to parent buffer in _http_request Fix incomplete get handling via HTTP compat bump and don't write failed retry streams to parent buffer in _http_request Nov 17, 2021
@IanButterworth
Copy link
Contributor Author

I assume the bors failure is something else?

@mattBrzezinski
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 17, 2021
@bors
Try #516:
54e0037
@bors
Copy link
Contributor

bors bot commented Nov 17, 2021

try

Build failed:

@IanButterworth
Copy link
Contributor Author

@omus Matt recommended I check if you know what's up with bors here?

@omus
Copy link
Member

omus commented Nov 17, 2021

@omus Matt recommended I check if you know what's up with bors here?

Ah, your move of write(response_stream, buffer) is the source of the problem. We need to write out the streamed data to response_stream during a failure otherwise the error is lost. That's exactly what this error is trying to communicate:

┌ Error: Internal Error: provided body is empty while the reported content-length is non-zero
└ @ AWS.AWSExceptions ~/work/AWS.jl/AWS.jl/src/AWSExceptions.jl:92

@omus
Copy link
Member

omus commented Nov 17, 2021

However, there was also an AWS.jl bug that this PR fixes, where each failed retry was being passed to the response_stream.

To fix this you may need to make buffer outside of the retry block and have an outer try/finally which writes the final response to the response_stream (a test should be added for this).

A possible better solution could be to not have HTTP.request throw exceptions. You'd need to throw an exception on the last attempt though.

@omus
Copy link
Member

omus commented Nov 17, 2021

bors try

bors bot added a commit that referenced this pull request Nov 17, 2021
@bors
Try #516:
e4def02
@bors
Copy link
Contributor

bors bot commented Nov 17, 2021

try

Build succeeded:

@IanButterworth
Copy link
Contributor Author

Great. Thanks @omus

omus
omus requested changes Nov 17, 2021
View reviewed changes
Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

I'd like to see a test for the EOFError behaviour. Otherwise these changes look good

@IanButterworth
Copy link
Contributor Author

IanButterworth commented Nov 17, 2021 •
edited
Loading

Ok. I'm not familiar with the mocking system here. Could you point me to a similar test?

Update: I think I figured out a test. Just added

@ericphanson
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 18, 2021
@bors
Try #516:
008ba2d
@bors
Copy link
Contributor

bors bot commented Nov 18, 2021

try

Build failed:

@ericphanson
Copy link
Member

bors try

@ericphanson
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 23, 2021
@bors
Try #516:
9555439
@bors
Copy link
Contributor

bors bot commented Nov 23, 2021

try

Build succeeded:

@omus
Copy link
Member

omus commented Nov 24, 2021 •
edited
Loading

If this PR requires further changes, I'd greatly appreciate code suggestions. I'm keen for this problem to be fixed and released.

I'll try to help push this along today

omus
omus reviewed Nov 24, 2021
View reviewed changes
test/issues.jl Outdated Show resolved Hide resolved
test/issues.jl Outdated Show resolved Hide resolved
src/utilities/request.jl Outdated Show resolved Hide resolved
src/utilities/request.jl Show resolved Hide resolved
@IanButterworth
Copy link
Contributor Author

Thanks @omus I just pushed the easy suggestions. Feel free to push the bigger change here, or in a following PR, whichever you prefer

@ericphanson
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 24, 2021
@bors
Try #516:
02ba9d4
@bors
Copy link
Contributor

bors bot commented Nov 24, 2021

try

Build failed:

@ericphanson
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 24, 2021
@bors
Try #516:
4231518
@bors
Copy link
Contributor

bors bot commented Nov 24, 2021

try

Build failed:

@IanButterworth
Copy link
Contributor Author

AWS was having a bad day, I think the error above is unrelated and cleared now

Load Credentials: Test Failed at /home/runner/work/AWS.jl/AWS.jl/test/AWSCredentials.jl:8
  Expression: e.code in [["AccessDenied", "EntityAlreadyExists"];]
   Evaluated: "400" in ["AccessDenied", "EntityAlreadyExists"]
Stacktrace:
 [1] top-level scope at /home/runner/work/AWS.jl/AWS.jl/test/AWSCredentials.jl:8
 [2] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1107
 [3] top-level scope at /home/runner/work/AWS.jl/AWS.jl/test/AWSCredentials.jl:17

omus
omus reviewed Nov 24, 2021
View reviewed changes
src/utilities/downloads_backend.jl Outdated Show resolved Hide resolved
src/utilities/downloads_backend.jl Outdated Show resolved Hide resolved
src/utilities/downloads_backend.jl Outdated Show resolved Hide resolved
src/utilities/request.jl Outdated Show resolved Hide resolved
src/utilities/request.jl Outdated Show resolved Hide resolved
omus
omus approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

I'm good with to proceed with these changes now. We can attempt https://github.com/JuliaCloud/AWS.jl/pull/516/files#r756212793 as a follow up

@omus
Copy link
Member

omus commented Nov 24, 2021

bors try

bors bot added a commit that referenced this pull request Nov 24, 2021
@bors
Try #516:
fc9a5b6
@bors
Copy link
Contributor

bors bot commented Nov 24, 2021

try

Build succeeded:

@omus
Copy link
Member

omus commented Nov 25, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 25, 2021

Build succeeded:

@bors bors bot merged commit b91dce6 into JuliaCloud:master Nov 25, 2021
@omus omus mentioned this pull request Nov 25, 2021
Merged
@IanButterworth IanButterworth deleted the ib/fix_retry_stream branch November 25, 2021 19:20
@omus omus mentioned this pull request Nov 25, 2021
Closed
bors bot added a commit that referenced this pull request Nov 30, 2021
@bors @omus
Merge #522
5abec75 
522: Refactor requests write to `response_stream` once r=omus a=omus

Follow up to: #516 (comment)

By making the repeat try/catch the innermost one we can eliminate having to keep track of the number of attempts.

Co-authored-by: Curtis Vogt <[email protected]>
@IanButterworth IanButterworth mentioned this pull request Mar 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omus omus omus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IanButterworth @ericphanson @mattBrzezinski @omus