Open backus opened 8 years ago
An empty string is definitely unexpected and undesirable in this case, however I don't think it makes sense to eagerly read bodies clients aren't interested in.
I think it would make sense to raise an exception in this case.
@tarcieri Ok so the issue in my case was that we have an integration test like this:
# creating the resource
response1 = client.post(...)
# getting the resource
response2 = client.get(response1.headers['Content-Location'])
expect(response1.body.to_s).to eql(response2.body.to_s)
Are you saying that this shouldn't be allowed behavior or should raise an exception?
I think if you make a request, do not consume the body, make another request with the same client, and then try to consume the body for the original request, it should raise an exception.
If you would like to consume the body for the original request, it should happen before you make a subsequent request, IMO. Otherwise the client needs to both consume and hang onto any response bodies simply because you might consume them at some point in the future.
Users uninterested in the response bodies would probably file a bug for that, calling it a "memory leak", and I would agree with them.
@tarcieri I think that sounds surprising to me because my impression of HTTP.rb was that it aimed to provide an immutable and chainable interface for performing requests. While I agree that raising an error would be better than what I've reported it seems like a bandaid. My impression was that the chaining methods were providing me new instances without shared mutable state.
Basically, just like how these two return values (ret1
and ret2
) should not be able to mutate each other
client = HTTP.accept('application/blah')
ret1 = client.accept("application/json")
ret2 = client.basic_auth(user: 'foo', password: 'bar')
it seems like these two return values should not be allowed to mutate each other:
client = HTTP.accept('application/blah')
ret1 = client.get("https://google.com")
ret2 = client.get("https://github.com")
The usage of either of these two response objects should not alter the behavior/state of the other.
At the very least it would be nice if I could have something like response.finalize
that I can call and get a simple immutable response wrapper that isn't tied to a connection object.
Calling #to_s
on the body prior to making another connection will accomplish that.
Yes, I will admit this behavior is a little bit surprising even to me.
there's helper for that too:
client = HTTP.accept('application/blah')
ret1 = client.get("https://google.com").flush
ret2 = client.get("https://github.com").flush
ret1.to_s[0..42]
# => "<HTML><HEAD><meta http-equiv=\"content-type\""
ret2.to_s[0..42]
# => " "
flush
consumes body and returns response itself
Would it introduce performance implications and/or breaking changes if each response object had a separate connection object?
Also thank you @ixti that is helpful
It will be (probably) a breaking change. And yeah, I guess it will make it easier to shoot the foot for one. But in general I think it's pretty doable and might become a pretty good improvement.
I think holding on to the last pending response in HTTP::Connection
could solve this problem.
The connection already tracks if a response is pending (i.e. the body is not read yet) and has enough information to release its reference to the response to avoid a memory leak (#finish_response
, #close
).
If @pending_response
(or an additional instance variable) were to contain an actual HTTP::Response
instead of a bool the connection could call #flush
on it when it's asked to send a new request.
If I use
HTTP
to perform two requests in a row and I don't doresponse.body.to_s
on the first then the body string for the first response will be overwritten with""
.Here is a script that demonstrates the bug:
Output when I run the script: