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

Fix chance of incomplete read in partial response stream reading #778

Merged
merged 4 commits into from
Nov 16, 2021

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Nov 15, 2021

This appears to fix a bug seen over in JuliaCloud/AWS.jl#515 where a 206 partial request response has the correct Content-Length in the header but HTTP.jl returns fewer bytes with no error.

The bug is seen most frequently when running many @async HTTP.requests each requesting a data Range on slower internet connections.

The problem appears to be that a simple write(response_stream, http) bypassed the ntoread handling here that ensures successive reads were done if the promised content length wasn't fully retrieved the first time, which allows for the buffer to populate slowly on slower connections, so it needs to be write(response_stream, read(http)) instead

@fredrikekre
Copy link
Member

Doesn't this "undo" the streaming feature?

@IanButterworth
Copy link
Member Author

I see. Perhaps there's something else needed to prevent the write from finishing too early if more content is expected?

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2021

Codecov Report

Merging #778 (76b6fcd) into master (f359667) will decrease coverage by 0.87%.
The diff coverage is 64.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
- Coverage   77.47%   76.60%   -0.88%     
==========================================
  Files          38       38              
  Lines        2562     2573      +11     
==========================================
- Hits         1985     1971      -14     
- Misses        577      602      +25     
Impacted Files Coverage Δ
src/Streams.jl 93.18% <14.28%> (-3.27%) ⬇️
src/StreamRequest.jl 95.77% <100.00%> (+0.25%) ⬆️
src/HTTP.jl 76.59% <0.00%> (-19.15%) ⬇️
src/WebSockets.jl 85.80% <0.00%> (-4.52%) ⬇️
src/ConnectionPool.jl 80.71% <0.00%> (-0.90%) ⬇️

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 f359667...76b6fcd. Read the comment docs.

@IanButterworth
Copy link
Member Author

@fredrikekre @iamed2

Doesn't this "undo" the streaming feature?

I think I just updated the fix to allow for streaming? Could someone run the CI please 🙏🏻

@IanButterworth IanButterworth force-pushed the ib/fix_partial_response_stream branch from 21c1505 to c0ac2f3 Compare November 16, 2021 02:09
@IanButterworth IanButterworth force-pushed the ib/fix_partial_response_stream branch from 0d698cf to 76b6fcd Compare November 16, 2021 06:34
@IanButterworth
Copy link
Member Author

Tests passing on my fork IanButterworth#1

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

This LGTM; thanks @IanButterworth.

@quinnj quinnj merged commit 80815e3 into JuliaWeb:master Nov 16, 2021
@@ -303,12 +303,23 @@ function Base.unsafe_read(http::Stream, p::Ptr{UInt8}, n::UInt)
nothing
end

Base.readbytes!(http::Stream, buf::Base.BufferStream, n=bytesavailable(http)) = Base.readbytes!(http, buf.buffer, n)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you have to special case BufferStream and IOBuffer? Will this fail with other IO now?

IanButterworth added a commit to IanButterworth/HTTP.jl that referenced this pull request Nov 16, 2021
IanButterworth added a commit to IanButterworth/HTTP.jl that referenced this pull request Nov 16, 2021
IanButterworth added a commit to IanButterworth/HTTP.jl that referenced this pull request Nov 17, 2021
fredrikekre pushed a commit that referenced this pull request Nov 17, 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.

4 participants