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

response_stream breaks with IOBuffer() #543

Closed
staticfloat opened this issue Jun 11, 2020 · 8 comments · Fixed by #752 or #775
Closed

response_stream breaks with IOBuffer() #543

staticfloat opened this issue Jun 11, 2020 · 8 comments · Fixed by #752 or #775

Comments

@staticfloat
Copy link
Contributor

staticfloat commented Jun 11, 2020

Versions:

Julia 1.4.1 
HTTP.jl 0.8.15

Example:

using HTTP
io = IOBuffer(read=true, write=true)
HTTP.get("https://google.com", response_stream=io)

Which results in:

ERROR: ArgumentError: ensureroom failed, IOBuffer is not writeable
Stacktrace:
 [1] ensureroom_slowpath(::Base.GenericIOBuffer{Array{UInt8,1}}, ::UInt64) at ./iobuffer.jl:298
 [2] ensureroom at ./iobuffer.jl:320 [inlined]
 [3] unsafe_write(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Ptr{UInt8}, ::UInt64) at ./iobuffer.jl:414
 [4] unsafe_write at ./io.jl:593 [inlined]
 [5] write at ./io.jl:616 [inlined]
 [6] write(::Base.GenericIOBuffer{Array{UInt8,1}}, ::HTTP.Streams.Stream{HTTP.Messages.Response,HTTP.ConnectionPool.Transaction{MbedTLS.SSLContext}}) at ./io.jl:663
 [7] readbody at /home/sabae/.julia/packages/HTTP/BOJmV/src/StreamRequest.jl:146 [inlined]
 [8] macro expansion at /home/sabae/.julia/packages/HTTP/BOJmV/src/StreamRequest.jl:67 [inlined]
 [9] macro expansion at ./task.jl:334 [inlined]
 [10] request(::Type{StreamLayer{Union{}}}, ::HTTP.ConnectionPool.Transaction{MbedTLS.SSLContext}, ::HTTP.Messages.Request, ::Array{UInt8,1}; response_stream::Base.GenericIOBuffer{Array{UInt8,1}}, iofunction::Nothing, verbose::Int64, kw::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/StreamRequest.jl:56
 [11] request(::Type{ConnectionPoolLayer{StreamLayer{Union{}}}}, ::HTTP.URIs.URI, ::HTTP.Messages.Request, ::Array{UInt8,1}; proxy::Nothing, socket_type::Type{T} where T, reuse_limit::Int64, kw::Base.Iterators.Pairs{Symbol,Union{Nothing, Base.GenericIOBuffer{Array{UInt8,1}}},Tuple{Symbol,Symbol},NamedTuple{(:iofunction, :response_stream),Tuple{Nothing,Base.GenericIOBuffer{Array{UInt8,1}}}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/ConnectionRequest.jl:89
 [12] request(::Type{ExceptionLayer{ConnectionPoolLayer{StreamLayer{Union{}}}}}, ::HTTP.URIs.URI, ::Vararg{Any,N} where N; kw::Base.Iterators.Pairs{Symbol,Union{Nothing, Base.GenericIOBuffer{Array{UInt8,1}}},Tuple{Symbol,Symbol},NamedTuple{(:iofunction, :response_stream),Tuple{Nothing,Base.GenericIOBuffer{Array{UInt8,1}}}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/ExceptionRequest.jl:19
 [13] (::Base.var"#58#60"{Base.var"#58#59#61"{ExponentialBackOff,HTTP.RetryRequest.var"#2#3"{Bool,HTTP.Messages.Request},typeof(HTTP.request)}})(::Type{T} where T, ::Vararg{Any,N} where N; kwargs::Base.Iterators.Pairs{Symbol,Union{Nothing, Base.GenericIOBuffer{Array{UInt8,1}}},Tuple{Symbol,Symbol},NamedTuple{(:iofunction, :response_stream),Tuple{Nothing,Base.GenericIOBuffer{Array{UInt8,1}}}}}) at ./error.jl:288
 [14] #request#1 at /home/sabae/.julia/packages/HTTP/BOJmV/src/RetryRequest.jl:44 [inlined]
 [15] request(::Type{MessageLayer{RetryLayer{ExceptionLayer{ConnectionPoolLayer{StreamLayer{Union{}}}}}}}, ::String, ::HTTP.URIs.URI, ::Array{Pair{SubString{String},SubString{String}},1}, ::Array{UInt8,1}; http_version::VersionNumber, target::String, parent::HTTP.Messages.Response, iofunction::Nothing, kw::Base.Iterators.Pairs{Symbol,Base.GenericIOBuffer{Array{UInt8,1}},Tuple{Symbol},NamedTuple{(:response_stream,),Tuple{Base.GenericIOBuffer{Array{UInt8,1}}}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/MessageRequest.jl:51
 [16] request(::Type{BasicAuthLayer{MessageLayer{RetryLayer{ExceptionLayer{ConnectionPoolLayer{StreamLayer{Union{}}}}}}}}, ::String, ::HTTP.URIs.URI, ::Array{Pair{SubString{String},SubString{String}},1}, ::Array{UInt8,1}; kw::Base.Iterators.Pairs{Symbol,Any,Tuple{Symbol,Symbol},NamedTuple{(:response_stream, :parent),Tuple{Base.GenericIOBuffer{Array{UInt8,1}},HTTP.Messages.Response}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/BasicAuthRequest.jl:28
 [17] request(::Type{RedirectLayer{BasicAuthLayer{MessageLayer{RetryLayer{ExceptionLayer{ConnectionPoolLayer{StreamLayer{Union{}}}}}}}}}, ::String, ::HTTP.URIs.URI, ::Array{Pair{SubString{String},SubString{String}},1}, ::Array{UInt8,1}; redirect_limit::Int64, forwardheaders::Bool, kw::Base.Iterators.Pairs{Symbol,Base.GenericIOBuffer{Array{UInt8,1}},Tuple{Symbol},NamedTuple{(:response_stream,),Tuple{Base.GenericIOBuffer{Array{UInt8,1}}}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/RedirectRequest.jl:24
 [18] request(::String, ::String, ::Array{Pair{SubString{String},SubString{String}},1}, ::Array{UInt8,1}; headers::Array{Pair{SubString{String},SubString{String}},1}, body::Array{UInt8,1}, query::Nothing, kw::Base.Iterators.Pairs{Symbol,Base.GenericIOBuffer{Array{UInt8,1}},Tuple{Symbol},NamedTuple{(:response_stream,),Tuple{Base.GenericIOBuffer{Array{UInt8,1}}}}}) at /home/sabae/.julia/packages/HTTP/BOJmV/src/HTTP.jl:314
 [19] #get#12 at /home/sabae/.julia/packages/HTTP/BOJmV/src/HTTP.jl:391 [inlined]
 [20] top-level scope at REPL[12]:3
@omus
Copy link
Contributor

omus commented Jun 18, 2021

Using Julia 1.6.1 and HTTP 0.9.10 I no longer see this error but instead get an unusable stream:

julia> using HTTP

julia> io = IOBuffer(read=true, write=true);

julia> HTTP.get("https://google.com", response_stream=io)
HTTP.Messages.Response:
"""
HTTP/1.1 200 OK
Date: Fri, 18 Jun 2021 17:56:00 GMT
Expires: -1
Cache-Control: private, max-age=0
Content-Type: text/html; charset=ISO-8859-1
P3P: CP="This is not a P3P policy! See g.co/p3phelp for more info."
Server: gws
X-XSS-Protection: 0
X-Frame-Options: SAMEORIGIN
Set-Cookie: 1P_JAR=2021-06-18-17; expires=Sun, 18-Jul-2021 17:56:00 GMT; path=/; domain=.google.com; Secure
Set-Cookie: NID=217=Sz2svs1yEqHxxLW_Ut5xUk03LSxp6zEOglskwqmOvMIOhAvzJE7x9dX-2XfSdzqAU1f4h-gfZ5zmBsKWFI-ms3QeP6jJnQHynhDMNohL5D6bNftc-pIKGXUkFHng0aWkPK905lSCrUOoi2nnV5ICcTXnjuRdLYAMUJsDLv9076w; expires=Sat, 18-Dec-2021 17:56:00 GMT; path=/; domain=.google.com; HttpOnly
Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
Accept-Ranges: none
Vary: Accept-Encoding
Transfer-Encoding: chunked

[Message Body was streamed]"""

julia> isopen(io)
false

julia> read(io, String)
ERROR: ArgumentError: read failed, IOBuffer is not readable
Stacktrace:
 [1] _throw_not_readable()
   @ Base ./iobuffer.jl:160
 [2] unsafe_read(from::IOBuffer, p::Ptr{UInt8}, nb::UInt64)
   @ Base ./iobuffer.jl:164
 [3] unsafe_read
   @ ./io.jl:722 [inlined]
 [4] read!
   @ ./io.jl:734 [inlined]
 [5] read
   @ ./iobuffer.jl:469 [inlined]
 [6] read(s::IOBuffer, #unused#::Type{String})
   @ Base ./io.jl:966
 [7] top-level scope
   @ REPL[11]:1

@c42f
Copy link
Contributor

c42f commented Sep 1, 2021

but instead get an unusable stream

This is because HTTP.jl closees the stream:

close(response_stream)

whereas it should call closewrite()... but Base.closewrite() won't exist until Julia 1.8.

@c42f
Copy link
Contributor

c42f commented Sep 1, 2021

Actually — should HTTP.jl even be calling close at all? It seems a bit heavy handed and unnecessary. Given that HTTP.request is a blocking operation, the caller should easily be able to close the stream themselves:

HTTP.get("http://example.com", response_stream=io)
close(io)

The close() was added in 422c8f5 but it's not really clear why.

@quinnj
Copy link
Member

quinnj commented Sep 1, 2021

I agree that we probably dont' need to call close there; I remember wondering why we did that as well at some point, but yeah, I don't think we should.

@omus
Copy link
Contributor

omus commented Sep 7, 2021

I agree that we probably dont' need to call close there; I remember wondering why we did that as well at some point, but yeah, I don't think we should.

I think the reason the stream is closed is because Base.BufferedStream blocks reading the entire stream unless the I/O type is closed. I don't think we should cater to this internal I/O type though and closing the stream should be handled externally.

@omus
Copy link
Contributor

omus commented Sep 7, 2021

whereas it should call closewrite()... but Base.closewrite() won't exist until Julia 1.8.

Is there a reason we even need use closewrite? As HTTP.jl blocks until the contents have been read into the I/O stream there isn't a reason to close the write capabilities of the I/O stream to signal that content has been written out. Only if HTTP.jl supported asynchronous reads would this matter.

@c42f
Copy link
Contributor

c42f commented Sep 8, 2021

Is there a reason we even need use closewrite

No. That's why I was questioning any kind of close in my second comment.

Only if HTTP.jl supported asynchronous reads would this matter.

Exactly 👍

@c42f
Copy link
Contributor

c42f commented Sep 8, 2021

I don't think we should cater to this internal I/O type though

I thought so too, but I've just spotted the following example in the HTTP.request() documentation which seems like quite a sensible use case:

io = Base.BufferStream()
@async while !eof(io)
    bytes = readavailable(io)
    println("GET data: \$bytes")
end
r = HTTP.request("GET", "http://httpbin.org/get", response_stream=io)
close(io)

Luckily the docs already suggest that the caller should call close() here :-)

c42f added a commit that referenced this issue Sep 8, 2021
This allows a plain `IOBuffer` to be used with `response_stream`.

It seems unnecessary to be calling `close(response_stream)` inside
HTTP.request() - given that HTTP.request is a blocking operation, the
caller can easily close the stream themselves after `HTTP.request()`
returns.

This could be considered a breaking change for users who use the
internal IO type Base.BufferStream with the response_stream keyword.

Fixes #543
@c42f c42f closed this as completed in #752 Sep 8, 2021
c42f added a commit that referenced this issue Sep 8, 2021
This allows a plain `IOBuffer` to be used with `response_stream`.

It seems unnecessary to be calling `close(response_stream)` inside
HTTP.request() - given that HTTP.request is a blocking operation, the
caller can easily close the stream themselves after `HTTP.request()`
returns.

This could be considered a breaking change for users who use the
internal IO type Base.BufferStream with the response_stream keyword.

Fixes #543
c42f added a commit that referenced this issue Oct 5, 2021
This allows a plain `IOBuffer` to be used with `response_stream`.

It seems unnecessary to be calling `close(response_stream)` inside
HTTP.request() - given that HTTP.request is a blocking operation, the
caller can easily close the stream themselves after `HTTP.request()`
returns.

This could be considered a breaking change for users who use the
internal IO type Base.BufferStream with the response_stream keyword.

Fixes #543
@c42f c42f reopened this Oct 5, 2021
quinnj added a commit that referenced this issue Dec 15, 2021
This allows a plain `IOBuffer` to be used with `response_stream`.

It seems unnecessary to be calling `close(response_stream)` inside
HTTP.request() - given that HTTP.request is a blocking operation, the
caller can easily close the stream themselves after `HTTP.request()`
returns.

This could be considered a breaking change for users who use the
internal IO type Base.BufferStream with the response_stream keyword.

Fixes #543

Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
quinnj added a commit that referenced this issue Mar 10, 2022
This allows a plain `IOBuffer` to be used with `response_stream`.

It seems unnecessary to be calling `close(response_stream)` inside
HTTP.request() - given that HTTP.request is a blocking operation, the
caller can easily close the stream themselves after `HTTP.request()`
returns.

This could be considered a breaking change for users who use the
internal IO type Base.BufferStream with the response_stream keyword.

Fixes #543

Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants