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

back to io/wait #368

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2590ac2
reverting the previous io/wait commits as a first measure, and reprod…
Aug 8, 2016
5a25cb7
added helper methods to the response, which will help inferring the p…
Aug 13, 2016
b04a0db
added alternative debugger for ruby > 2.0
Aug 13, 2016
f5b22e5
enhanced the master branch #content_length implementation, regarding …
Aug 13, 2016
ca55acf
passing content length to response body; when set, read until length …
Aug 13, 2016
21f9d34
CI has old version of bundler, and I can't set the newest platforms..…
Aug 13, 2016
4a39a8b
tolerating the first timeout on read/write wait; like in the failing …
Aug 14, 2016
f79bf10
added general header error, left the existing custom one for back-comp
Aug 19, 2016
f0f0231
wrong format for content length doesn't raise error, which doesn't pl…
Aug 23, 2016
48c3e85
corrected some faults coming from latest rebase
Aug 24, 2016
93c9ce9
explicit nil
Aug 24, 2016
73f0816
do not needlessly call the method, use the ivar
Aug 24, 2016
00279ae
Headers#get always returns an array, so work only with that; also, us…
Aug 24, 2016
9a2dea1
use Headers#include? to reduce allocations, add header conditions tog…
Aug 24, 2016
c36d357
according to RFC, response is only chunked only if the transfer encod…
Aug 24, 2016
83f3b32
added comment for future refactoring
Aug 24, 2016
08ec27d
removed the extra include? call over there
Aug 30, 2016
9c18fe3
rubocoping
Aug 30, 2016
6a13e2a
rubocop <3 ...
Jan 30, 2017
108e127
reintegrated kwargs in the body API, and passing the content length down
Feb 3, 2017
607e42b
rubo rubo rubocop
Feb 3, 2017
4a90c6c
rubocop, make up your mind...
Feb 3, 2017
21c0f60
removing deprecation
Feb 3, 2017
7fc577b
removed tests with deprecated error..
Feb 3, 2017
5b19215
rubocop again...
Feb 3, 2017
8d84978
more ruboscosps...
Feb 3, 2017
0346723
Merge branch 'master' into issue_357
HoneyryderChuck Feb 3, 2017
a77e80e
Merge branch 'master' into issue_357
HoneyryderChuck Feb 6, 2017
a6afeeb
rbc
Feb 6, 2017
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
7 changes: 5 additions & 2 deletions lib/http/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ class StateError < ResponseError; end
# Generic Timeout error
class TimeoutError < Error; end

# Header name is invalid
class InvalidHeaderNameError < Error; end
# Header value is of unexpected format (similar to Net::HTTPHeaderSyntaxError)
class HeaderError < Error; end

# @deprecated will be removed in v3.0.0
Copy link
Member

Choose a reason for hiding this comment

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

As we're gonna release this PR as part of v3.0.0 it worth to change this to be will be removed in v3.1.0

Copy link
Author

Choose a reason for hiding this comment

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

But if it is to be removed in 3.0.0 and this PR is about 3.0.0, then why not just go and remove support? I'd save this tweak to upstream though, it's beyond the scope of the PR IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Mmm. I guess I'm agree. Just remove this deprecated class then.

InvalidHeaderNameError = HeaderError
end
4 changes: 2 additions & 2 deletions lib/http/headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ def coerce(object)
# Transforms `name` to canonical HTTP header capitalization
#
# @param [String] name
# @raise [InvalidHeaderNameError] if normalized name does not
# @raise [HeaderError] if normalized name does not
# match {HEADER_NAME_RE}
# @return [String] canonical HTTP header name
def normalize_header(name)
Expand All @@ -206,7 +206,7 @@ def normalize_header(name)

return normalized if normalized =~ COMPLIANT_NAME_RE

raise InvalidHeaderNameError, "Invalid HTTP header field name: #{name.inspect}"
raise HeaderError, "Invalid HTTP header field name: #{name.inspect}"
end
end
end
16 changes: 15 additions & 1 deletion lib/http/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,13 @@ def flush
# (not an integer, e.g. empty string or string with non-digits).
# @return [Integer] otherwise
def content_length
# http://greenbytes.de/tech/webdav/rfc7230.html#rfc.section.3.3.3
# Clause 3: "If a message is received with both a Transfer-Encoding
# and a Content-Length header field, the Transfer-Encoding overrides the Content-Length.
return nil if @headers.include?(Headers::TRANSFER_ENCODING)

value = @headers[Headers::CONTENT_LENGTH]
return unless value
return nil unless value

begin
Integer(value)
Expand Down Expand Up @@ -132,6 +137,15 @@ def cookies
end
end

def chunked?
return false unless @headers.include?(Headers::TRANSFER_ENCODING)

encoding = @headers.get(Headers::TRANSFER_ENCODING)

# TODO: "chunked" is frozen in the request writer. How about making it accessible?
encoding.last == "chunked"
end

# Parse response body with corresponding MIME type adapter.
#
# @param [#to_s] as Parse as given MIME type
Expand Down
5 changes: 4 additions & 1 deletion lib/http/response/body.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,10 @@ def to_s
@streaming = false
@contents = String.new("").force_encoding(encoding)

while (chunk = @stream.readpartial)
length = @length || Float::INFINITY
Copy link
Author

Choose a reason for hiding this comment

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

@tarcieri the purpose of this variable got lost in the rebase. I was passing the content length (when available) to the body to prevent mismatches and eventual infinite loops. But the initialize signature changed. I leave the decision to you to take advantage of this or remove it.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should follow your initial idea of limiting amount of data to given Content-Length when available and should be respected according to RFC.

Copy link
Author

Choose a reason for hiding this comment

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

@ixti agree, but how should I "import" the length? The initializer signature changed, and I lost my assumptions. Just in case this PR takes again longer than planned: what do you suggest as the long term way to pass the content length from the response to the body?

When I implemented it, I changed the signature to use kwargs and added the encoding and content length as optional parameters. Can I go back to that strategy? as @tarcieri said, this is anyway planned to a major bump, so in theory I could.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, go ahead. I'm fine with keyword args. And it won't take long this time, I promise - will merge as soon as it's ready :D before this weekend.

Copy link
Author

Choose a reason for hiding this comment

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

will merge as soon as it's ready :D before this weekend.

Famous last words :D

I have a tentative commit here which I don't know if I'll include: I was going to make Response#content_length return Float::INFINITY when header not explicitly set. It's a bit avant-garde, since it's not completely correct, as 304 and 204 technically have content length 0. This could be arranged, though. This would make the method semantically more correct, but it's public API that could break some clients. Which could be ok, as it's v3, so breaking is acceptable if justified.

I pass the potato to you.


while (length > 0 && chunk = @stream.readpartial)
length -= chunk.bytesize
@contents << chunk.force_encoding(encoding)
end
rescue
Expand Down
24 changes: 18 additions & 6 deletions lib/http/timeout/per_operation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,29 +59,41 @@ def write(data)
else
# Read data from the socket
def readpartial(size)
timeout = false
loop do
result = @socket.read_nonblock(size, :exception => false)

return :eof if result.nil?
return result if result != :wait_readable

unless IO.select([@socket], nil, nil, read_timeout)
raise TimeoutError, "Read timed out after #{read_timeout} seconds"
end
raise TimeoutError, "Read timed out after #{read_timeout} seconds" if timeout
# marking the socket for timeout. Why is this not being raised immediately?
# it seems there is some race-condition on the network level between calling
# #read_nonblock and #wait_readable, in which #read_nonblock signalizes waiting
# for reads, and when waiting for x seconds, it returns nil suddenly without completing
# the x seconds. In a normal case this would be a timeout on wait/read, but it can
# also mean that the socket has been closed by the server. Therefore we "mark" the
# socket for timeout and try to read more bytes. If it returns :eof, it's all good, no
# timeout. Else, the first timeout was a proper timeout.
# This hack has to be done because io/wait#wait_readable doesn't provide a value for when
# the socket is closed by the server, and HTTP::Parser doesn't provide the limit for the chunks.
timeout = true unless @socket.to_io.wait_readable(read_timeout)
end
end

# Write data to the socket
def write(data)
timeout = false
loop do
result = @socket.write_nonblock(data, :exception => false)
return result unless result == :wait_writable

unless IO.select(nil, [@socket], nil, write_timeout)
raise TimeoutError, "Write timed out after #{write_timeout} seconds"
end
raise TimeoutError, "Write timed out after #{write_timeout} seconds" if timeout

timeout = true unless @socket.to_io.wait_writable(write_timeout)
end
end

end
end
end
Expand Down
9 changes: 9 additions & 0 deletions spec/lib/http/response_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,13 @@
expect(response.connection).to eq connection
end
end

describe "#chunked?" do
subject { response }
context "when encoding is set to chunked" do
let(:headers) { {"Transfer-Encoding" => "chunked"} }
it { is_expected.to be_chunked }
end
it { is_expected.not_to be_chunked }
end
end