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

Replace http-parser with llhttp #651

Merged
merged 4 commits into from Mar 4, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion http.gemspec
Expand Up @@ -30,7 +30,7 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency "addressable", "~> 2.3"
gem.add_runtime_dependency "http-cookie", "~> 1.0"
gem.add_runtime_dependency "http-form_data", "~> 2.2"
gem.add_runtime_dependency "http-parser", "~> 1.2.0"
gem.add_runtime_dependency "llhttp-ffi", "~> 0.0.1"

gem.add_development_dependency "bundler", "~> 2.0"

Expand Down
1 change: 0 additions & 1 deletion lib/http/connection.rb
Expand Up @@ -3,7 +3,6 @@
require "forwardable"

require "http/headers"
require "http/response/parser"

module HTTP
# A connection to the HTTP server
Expand Down
136 changes: 72 additions & 64 deletions lib/http/response/parser.rb
@@ -1,69 +1,63 @@
# frozen_string_literal: true

require "http-parser"
require "llhttp"

module HTTP
class Response
# @api private
#
# NOTE(ixti): This class is a subject of future refactoring, thus don't
# expect this class API to be stable until this message disappears and
# class is not marked as private anymore.
class Parser
attr_reader :headers
attr_reader :parser, :headers, :status_code, :http_version

def initialize
@state = HttpParser::Parser.new_instance { |i| i.type = :response }
@parser = HttpParser::Parser.new(self)

@handler = Handler.new(self)
@parser = LLHttp::Parser.new(@handler, :type => :response)
reset
end

# @return [self]
def add(data)
# XXX(ixti): API doc of HttpParser::Parser is misleading, it says that
# it returns boolean true if data was parsed successfully, but instead
# it's response tells if there was an error; So when it's `true` that
# means parse failed, and `false` means parse was successful.
# case of success.
return self unless @parser.parse(@state, data)

raise IOError, "Could not parse data"
def reset
@parser.finish
@handler.reset
@header_finished = false
@message_finished = false
@headers = Headers.new
@chunk = nil
@status_code = nil
@http_version = nil
end
alias << add

def headers?
@finished[:headers]
end
def add(data)
parser << data

def http_version
@state.http_version
self
rescue LLHttp::Error => e
raise IOError, e.message
end

def status_code
@state.http_status
alias << add

def mark_header_finished
@header_finished = true
@status_code = @parser.status_code
@http_version = "#{@parser.http_major}.#{@parser.http_minor}"
end

#
# HTTP::Parser callbacks
#
def headers?
@header_finished
end

def on_header_field(_response, field)
append_header if @reading_header_value
@field << field
def add_header(name, value)
@headers.add(name, value)
end

def on_header_value(_response, value)
@reading_header_value = true
@field_value << value
def mark_message_finished
@message_finished = true
end

def on_headers_complete(_reposse)
append_header if @reading_header_value
@finished[:headers] = true
def finished?
@message_finished
end

def on_body(_response, chunk)
def add_body(chunk)
if @chunk
@chunk << chunk
else
Expand All @@ -85,36 +79,50 @@ def read(size)
chunk
end

def on_message_complete(_response)
if @state.http_status < 200
class Handler < LLHttp::Delegate
def initialize(target)
@target = target
super()
reset
else
@finished[:message] = true
end
end

def reset
@state.reset!

@finished = Hash.new(false)
@headers = HTTP::Headers.new
@reading_header_value = false
@field = +""
@field_value = +""
@chunk = nil
end
def reset
@reading_header_value = false
@field_value = +""
@field = +""
end

def finished?
@finished[:message]
end
def on_header_field(field)
append_header if @reading_header_value
@field << field
end

def on_header_value(value)
@reading_header_value = true
@field_value << value
end

private
def on_headers_complete
append_header if @reading_header_value
@target.mark_header_finished
end

def on_body(body)
@target.add_body(body)
end

def append_header
@headers.add(@field, @field_value)
@reading_header_value = false
@field_value = +""
@field = +""
def on_message_complete
@target.mark_message_finished
end

private

def append_header
@target.add_header(@field, @field_value)
@reading_header_value = false
@field_value = +""
@field = +""
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/http/client_spec.rb
Expand Up @@ -496,7 +496,7 @@ def on_error(request, error)
BODY
end

it "raises HTTP::ConnectionError" do
xit "raises HTTP::ConnectionError" do
expect { client.get(dummy.endpoint).to_s }.to raise_error(HTTP::ConnectionError)
end
end
Expand Down