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

Support Rack 3.x #399

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 2 additions & 2 deletions example/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ def call(env)
run SimpleAdapter.new
end
map '/files' do
run Rack::File.new('.')
run Rack::Files.new('.')
end
end

# You could also start the server like this:
#
# app = Rack::URLMap.new('/test' => SimpleAdapter.new,
# '/files' => Rack::File.new('.'))
# '/files' => Rack::Files.new('.'))
# Thin::Server.start('0.0.0.0', 3000, app)
#
2 changes: 1 addition & 1 deletion ext/thin_parser/common.rl
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
Method = ( upper | digit | safe ){1,20} >mark %request_method;

http_number = ( digit+ "." digit+ ) ;
HTTP_Version = ( "HTTP/" http_number ) >mark %http_version ;
HTTP_Version = ( "HTTP/" http_number ) >mark %request_http_version ;
Request_Line = ( Method " " Request_URI ("#" Fragment){0,1} " " HTTP_Version CRLF ) ;

field_name = ( token -- ":" )+ >start_field %write_field;
Expand Down
4 changes: 2 additions & 2 deletions ext/thin_parser/parser.c
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ case 13:
tr17:
#line 59 "parser.rl"
{
if (parser->http_version != NULL) {
parser->http_version(parser->data, PTR_TO(mark), LEN(mark, p));
if (parser->request_http_version != NULL) {
parser->request_http_version(parser->data, PTR_TO(mark), LEN(mark, p));
}
}
goto st14;
Expand Down
2 changes: 1 addition & 1 deletion ext/thin_parser/parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ typedef struct http_parser {
element_cb fragment;
element_cb request_path;
element_cb query_string;
element_cb http_version;
element_cb request_http_version;
element_cb header_done;

} http_parser;
Expand Down
6 changes: 3 additions & 3 deletions ext/thin_parser/parser.rl
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@
}
}

action http_version {
if (parser->http_version != NULL) {
parser->http_version(parser->data, PTR_TO(mark), LEN(mark, fpc));
action request_http_version {
if (parser->request_http_version != NULL) {
parser->request_http_version(parser->data, PTR_TO(mark), LEN(mark, fpc));
}
}

Expand Down
10 changes: 5 additions & 5 deletions ext/thin_parser/thin.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ static VALUE global_request_method;
static VALUE global_request_uri;
static VALUE global_fragment;
static VALUE global_query_string;
static VALUE global_http_version;
static VALUE global_request_http_version;
static VALUE global_content_length;
static VALUE global_http_content_length;
static VALUE global_request_path;
Expand Down Expand Up @@ -150,11 +150,11 @@ static void query_string(void *data, const char *at, size_t length)
rb_hash_aset(req, global_query_string, val);
}

static void http_version(void *data, const char *at, size_t length)
static void request_http_version(void *data, const char *at, size_t length)
{
VALUE req = (VALUE)data;
VALUE val = rb_str_new(at, length);
rb_hash_aset(req, global_http_version, val);
rb_hash_aset(req, global_request_http_version, val);
}

/** Finalizes the request header to have a bunch of stuff that's
Expand Down Expand Up @@ -237,7 +237,7 @@ VALUE Thin_HttpParser_alloc(VALUE klass)
hp->fragment = fragment;
hp->request_path = request_path;
hp->query_string = query_string;
hp->http_version = http_version;
hp->request_http_version = request_http_version;
hp->header_done = header_done;
thin_http_parser_init(hp);

Expand Down Expand Up @@ -401,7 +401,7 @@ void Init_thin_parser()
DEF_GLOBAL(request_uri, "REQUEST_URI");
DEF_GLOBAL(fragment, "FRAGMENT");
DEF_GLOBAL(query_string, "QUERY_STRING");
DEF_GLOBAL(http_version, "HTTP_VERSION");
DEF_GLOBAL(request_http_version, "thin.request_http_version");
DEF_GLOBAL(request_path, "REQUEST_PATH");
DEF_GLOBAL(content_length, "CONTENT_LENGTH");
DEF_GLOBAL(http_content_length, "HTTP_CONTENT_LENGTH");
Expand Down
2 changes: 1 addition & 1 deletion lib/rack/adapter/loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def self.for(name, options={})
return Merb::Rack::Application.new

when :file
return Rack::File.new(options[:chdir])
return Rack::Files.new(options[:chdir])

else
raise AdapterNotFound, "Adapter not found: #{name}"
Expand Down
2 changes: 1 addition & 1 deletion lib/rack/adapter/rails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def initialize(options = {})
load_application

@rails_app = self.class.rack_based? ? ActionController::Dispatcher.new : CgiApp.new
@file_app = Rack::File.new(::File.join(RAILS_ROOT, "public"))
@file_app = Rack::Files.new(::File.join(RAILS_ROOT, "public"))
end

def load_application
Expand Down
4 changes: 2 additions & 2 deletions lib/thin/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class Request
SERVER_NAME = 'SERVER_NAME'.freeze
REQUEST_METHOD = 'REQUEST_METHOD'.freeze
LOCALHOST = 'localhost'.freeze
HTTP_VERSION = 'HTTP_VERSION'.freeze
REQUEST_HTTP_VERSION = 'thin.request_http_version'.freeze
HTTP_1_0 = 'HTTP/1.0'.freeze
REMOTE_ADDR = 'REMOTE_ADDR'.freeze
CONTENT_LENGTH = 'CONTENT_LENGTH'.freeze
Expand Down Expand Up @@ -114,7 +114,7 @@ def persistent?
# Clients and servers SHOULD NOT assume that a persistent connection
# is maintained for HTTP versions less than 1.1 unless it is explicitly
# signaled. (http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html)
if @env[HTTP_VERSION] == HTTP_1_0
if @env[REQUEST_HTTP_VERSION] == HTTP_1_0
@env[CONNECTION] =~ KEEP_ALIVE_REGEXP

# HTTP/1.1 client intends to maintain a persistent connection unless
Expand Down
52 changes: 50 additions & 2 deletions lib/thin/response.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,52 @@
module Thin
# A response sent to the client.
class Response
class Stream
def initialize(writer)
@read_closed = true
@write_closed = false
@writer = writer
end

def read(length = nil, outbuf = nil)
raise ::IOError, 'not opened for reading' if @read_closed
end

def write(chunk)
raise ::IOError, 'not opened for writing' if @write_closed

@writer.call(chunk)
end

alias :<< :write

def close
@read_closed = @write_closed = true

nil
end

def closed?
@read_closed && @write_closed
end

def close_read
@read_closed = true

nil
end

def close_write
@write_closed = true

nil
end

def flush
self
end
end

CONNECTION = 'Connection'.freeze
CLOSE = 'close'.freeze
KEEP_ALIVE = 'keep-alive'.freeze
Expand Down Expand Up @@ -86,14 +132,16 @@ def close
# Yields each chunk of the response.
# To control the size of each chunk
# define your own +each+ method on +body+.
def each
def each(&block)
yield head

unless @skip_body
if @body.is_a?(String)
yield @body
else
elsif @body.respond_to?(:each)
@body.each { |chunk| yield chunk }
else
@body.call(Stream.new(block))
end
end
end
Expand Down
5 changes: 2 additions & 3 deletions spec/request/parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
request = R("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n")
expect(request.env['SERVER_PROTOCOL']).to eq('HTTP/1.1')
expect(request.env['REQUEST_PATH']).to eq('/')
expect(request.env['HTTP_VERSION']).to eq('HTTP/1.1')
expect(request.env['thin.request_http_version']).to eq('HTTP/1.1')
Copy link
Collaborator

@ioquatix ioquatix Apr 1, 2024

Choose a reason for hiding this comment

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

IIRC, you should use SERVER_PROTOCOL for this. See https://github.com/rack/rack/blob/main/SPEC.rdoc for more details.

Copy link
Author

@lloeki lloeki Apr 3, 2024

Choose a reason for hiding this comment

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

Thin internals relied on HTTP_VERSION. Removing it would have broken things so I added this so as not to mess with things too much while I got clarification.

IIUC you're saying:

  • this logic here should rely on SERVER_PROTOCOL instead of HTTP_VERSION
  • thin.request_http_version can then disappear entirely

Correct?

expect(request.env['REQUEST_URI']).to eq('/')
expect(request.env['GATEWAY_INTERFACE']).to eq('CGI/1.2')
expect(request.env['REQUEST_METHOD']).to eq('GET')
Expand Down Expand Up @@ -195,7 +195,6 @@
expect(request.env['PATH_INFO']).to eq('/hi')
expect(request.env['REQUEST_PATH']).to eq('/hi')
expect(request.env['REQUEST_URI']).to eq('/hi?qs')
expect(request.env['HTTP_VERSION']).to eq('HTTP/1.1')
expect(request.env['REQUEST_METHOD']).to eq('GET')
expect(request.env["rack.url_scheme"]).to eq('http')
expect(request.env['FRAGMENT'].to_s).to eq("f")
Expand Down Expand Up @@ -276,4 +275,4 @@

expect(request.env['REQUEST_URI']).to eq("/H%uFFFDhnchenbrustfilet")
end
end
end
12 changes: 6 additions & 6 deletions spec/request/persistent_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,30 @@
end

it "should not assume that a persistent connection is maintained for HTTP version 1.0" do
@request.env['HTTP_VERSION'] = 'HTTP/1.0'
@request.env['thin.request_http_version'] = 'HTTP/1.0'
expect(@request).not_to be_persistent
end

it "should assume that a persistent connection is maintained for HTTP version 1.0 when specified" do
@request.env['HTTP_VERSION'] = 'HTTP/1.0'
@request.env['thin.request_http_version'] = 'HTTP/1.0'
@request.env['HTTP_CONNECTION'] = 'Keep-Alive'
expect(@request).to be_persistent
end

it "should maintain a persistent connection for HTTP/1.1 client" do
@request.env['HTTP_VERSION'] = 'HTTP/1.1'
@request.env['thin.request_http_version'] = 'HTTP/1.1'
@request.env['HTTP_CONNECTION'] = 'Keep-Alive'
expect(@request).to be_persistent
end

it "should maintain a persistent connection for HTTP/1.1 client by default" do
@request.env['HTTP_VERSION'] = 'HTTP/1.1'
@request.env['thin.request_http_version'] = 'HTTP/1.1'
expect(@request).to be_persistent
end

it "should not maintain a persistent connection for HTTP/1.1 client when Connection header include close" do
@request.env['HTTP_VERSION'] = 'HTTP/1.1'
@request.env['thin.request_http_version'] = 'HTTP/1.1'
@request.env['HTTP_CONNECTION'] = 'close'
expect(@request).not_to be_persistent
end
end
end
2 changes: 1 addition & 1 deletion spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def failure_message_when_negated
class ValidateWithLint
def matches?(request)
@request = request
Rack::Lint.new(proc{[200, {'Content-Type' => 'text/html', 'Content-Length' => '0'}, []]}).call(@request.env)
Rack::Lint.new(proc{[200, {'content-type' => 'text/html', 'content-length' => '0'}, []]}).call(@request.env)
true
rescue Rack::Lint::LintError => e
@message = e.message
Expand Down
2 changes: 1 addition & 1 deletion thin.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Thin::GemSpec ||= Gem::Specification.new do |s|

s.required_ruby_version = '>= 1.8.5'

s.add_dependency 'rack', '>= 1', '< 3'
s.add_dependency 'rack', '>= 1', '< 4'
s.add_dependency 'eventmachine', '~> 1.0', '>= 1.0.4'
s.add_dependency 'daemons', '~> 1.0', '>= 1.0.9' unless Thin.win?

Expand Down
Loading