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

Fixed chunk writing if the data is bugger than our buffer (#251) #252

Merged
merged 1 commit into from
Sep 6, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions lib/http/request/writer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ class Writer
# Chunked transfer encoding
CHUNKED = "chunked".freeze

# End of a chunked transfer
CHUNKED_END = "#{ZERO}#{CRLF}#{CRLF}".freeze

# Types valid to be used as body source
VALID_BODY_TYPES = [String, NilClass, Enumerable]

Expand Down Expand Up @@ -40,7 +43,7 @@ def stream
# Send headers needed to connect through proxy
def connect_through_proxy
add_headers
@socket << join_headers
write(join_headers)
end

# Adds the headers to the header array for the given request body we are working
Expand All @@ -65,24 +68,35 @@ def send_request_header
add_headers
add_body_type_headers

@socket << join_headers
write(join_headers)
end

def send_request_body
if @body.is_a?(String)
@socket << @body
write(@body)
elsif @body.is_a?(Enumerable)
@body.each do |chunk|
@socket << chunk.bytesize.to_s(16) << CRLF
@socket << chunk << CRLF
write(chunk.bytesize.to_s(16) << CRLF)
write(chunk << CRLF)
end

@socket << ZERO << CRLF << CRLF
write(CHUNKED_END)
end
end

private

def write(data)
while data.present?
length = @socket.write(data)
if data.length > length
data = data[length..-1]
Copy link
Member

Choose a reason for hiding this comment

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

This will allocate every time. I wonder if it would be possible to use #slice! here to reduce allocations. Or maybe we just need real ByteBuffers 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We call this using @body as is, so we probably can't use slice!, unless we call dup beforehand.

Would need to benchmark to figure out if the optimization works out.

Copy link
Member

Choose a reason for hiding this comment

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

Since Ruby strings are CoW anyway, this usage is probably efficient

else
break
end
end
end

def validate_body_type!
return if VALID_BODY_TYPES.any? { |type| @body.is_a? type }
fail RequestError, "body of wrong type: #{@body.class}"
Expand Down
4 changes: 2 additions & 2 deletions lib/http/timeout/global.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def write(data)
reset_timer

begin
socket.write_nonblock(data)
return socket.write_nonblock(data)
rescue IO::WaitWritable
IO.select(nil, [socket], nil, time_left)
log_time
Expand Down Expand Up @@ -97,7 +97,7 @@ def write(data)

loop do
result = socket.write_nonblock(data, :exception => false)
break unless result == :wait_writable
return result unless result == :wait_writable

IO.select(nil, [socket], nil, time_left)
log_time
Expand Down
2 changes: 1 addition & 1 deletion lib/http/timeout/null.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def readpartial(size)

# Write to the socket
def write(data)
@socket << data
@socket.write(data)
end
alias_method :<<, :write

Expand Down
2 changes: 1 addition & 1 deletion lib/http/timeout/per_operation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def readpartial(size)
def write(data)
loop do
result = socket.write_nonblock(data, :exception => false)
break unless result == :wait_writable
return result unless result == :wait_writable

unless IO.select(nil, [socket], nil, write_timeout)
fail TimeoutError, "Read timed out after #{write_timeout} seconds"
Expand Down
4 changes: 2 additions & 2 deletions spec/lib/http/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,8 @@ def simple_response(body, status = 200)

allow(socket_spy).to receive(:close) { nil }
allow(socket_spy).to receive(:closed?) { true }
allow(socket_spy).to receive(:readpartial) { chunks.shift }
allow(socket_spy).to receive(:<<) { nil }
allow(socket_spy).to receive(:readpartial) { chunks[0] }
allow(socket_spy).to receive(:write) { chunks[0].length }

allow(TCPSocket).to receive(:open) { socket_spy }
end
Expand Down
26 changes: 26 additions & 0 deletions spec/lib/http_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,32 @@
expect(response.to_s.include?("json")).to be true
end
end

context "with a large request body" do
%w(global null per_operation).each do |timeout|
context "with a #{timeout} timeout" do
[16_000, 16_500, 17_000, 34_000, 68_000].each do |size|
[0, rand(0..100), rand(100..1000)].each do |fuzzer|
context "with a #{size} body and #{fuzzer} of fuzzing" do
let(:client) { HTTP.timeout(timeout, :read => 2, :write => 2, :connect => 2) }

let(:characters) { ("A".."Z").to_a }
let(:request_body) do
(size + fuzzer).times.map { |i| characters[i % characters.length] }.join
end

it "returns a large body" do
response = client.post("#{dummy.endpoint}/echo-body", :body => request_body)

expect(response.body.to_s).to eq(request_body)
expect(response.headers["Content-Length"].to_i).to eq(request_body.length)
end
end
end
end
end
end
end
end

describe ".via" do
Expand Down
5 changes: 5 additions & 0 deletions spec/support/dummy_server/servlet.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,5 +133,10 @@ def do_#{method.upcase}(req, res)
res["Set-Cookie"] = "foo=bar"
res.body = req.cookies.map { |c| [c.name, c.value].join ": " }.join("\n")
end

post "/echo-body" do |req, res|
res.status = 200
res.body = req.body
end
end
end