rack-devel archive mirror (unofficial) https://groups.google.com/group/rack-devel
 help / color / mirror / code / Atom feed
* Rack::Server patch
@ 2009-11-21  0:42 Yehuda Katz
  2009-11-21  0:46 ` Yehuda Katz
  2009-11-21  0:52 ` Yehuda Katz
  0 siblings, 2 replies; 6+ messages in thread
From: Yehuda Katz @ 2009-11-21  0:42 UTC (permalink / raw)
  To: rack-devel

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

Hey guys,

Carl and I spent a couple of day refactoring the code in bin/rackup into
Rack::Server. The main motivation for this was to enable Rails to ditch our
code in script/server and simply inherit from the requisite Rack code. I
think the code improvement speaks for itself. In the process of this work,
we also moved a few things out of Rackup into more usable locations, like
Rack::Handler.default (to get the handler that Rack will use if none is
specified) and Rack::Builder.parse_file (which we currently duplicate in
ActionDispatch).

You can check out the changes at github.com/carllerche/rack, and I have also
attached a patch. In addition to converting the rackup binary to a class, we
also wrote tests for each function of rackup, to be sure we wouldn't break
anything in the refactor. As a result, this patch now has tests for rackup!

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

[-- Attachment #2: Type: text/html, Size: 1047 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Rack::Server patch
  2009-11-21  0:42 Rack::Server patch Yehuda Katz
@ 2009-11-21  0:46 ` Yehuda Katz
  2009-11-21  0:52 ` Yehuda Katz
  1 sibling, 0 replies; 6+ messages in thread
From: Yehuda Katz @ 2009-11-21  0:46 UTC (permalink / raw)
  To: rack-devel


[-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --]

Patches attached.

[1] tests_patch.diff -- Adds tests for the functionality of rackup
[2] rack_server_patch.diff -- Moves Rack::Server into a separate object

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325


2009/11/20 Yehuda Katz <wycats@gmail.com>

> Hey guys,
>
> Carl and I spent a couple of day refactoring the code in bin/rackup into
> Rack::Server. The main motivation for this was to enable Rails to ditch our
> code in script/server and simply inherit from the requisite Rack code. I
> think the code improvement speaks for itself. In the process of this work,
> we also moved a few things out of Rackup into more usable locations, like
> Rack::Handler.default (to get the handler that Rack will use if none is
> specified) and Rack::Builder.parse_file (which we currently duplicate in
> ActionDispatch).
>
> You can check out the changes at github.com/carllerche/rack, and I have
> also attached a patch. In addition to converting the rackup binary to a
> class, we also wrote tests for each function of rackup, to be sure we
> wouldn't break anything in the refactor. As a result, this patch now has
> tests for rackup!
>
> Yehuda Katz
> Developer | Engine Yard
> (ph) 718.877.1325
>

[-- Attachment #1.2: Type: text/html, Size: 1635 bytes --]

[-- Attachment #2: tests_patch.diff --]
[-- Type: application/octet-stream, Size: 5366 bytes --]

diff --git a/bin/rackup b/bin/rackup
index 91abe17..3a90327 100755
--- a/bin/rackup
+++ b/bin/rackup
@@ -9,7 +9,7 @@ automatic = false
 server = nil
 env = "development"
 daemonize = false
-pid = nil
+pid = File.expand_path("rack.pid")
 options = {:Port => 9292, :Host => "0.0.0.0", :AccessLog => []}
 
 # Don't evaluate CGI ISINDEX parameters.
diff --git a/test/rackup/config.ru b/test/rackup/config.ru
new file mode 100644
index 0000000..2490c6e
--- /dev/null
+++ b/test/rackup/config.ru
@@ -0,0 +1,25 @@
+require "#{File.dirname(__FILE__)}/../testrequest"
+
+$stderr = StringIO.new
+
+class EnvMiddleware
+  def initialize(app)
+    @app = app
+  end
+  
+  def call(env)
+    if env["PATH_INFO"] == "/broken_lint"
+      return [200, {}, ["Broken Lint"]]
+    end
+
+    env["test.$DEBUG"]      = $DEBUG
+    env["test.$EVAL"]       = BUKKIT if defined?(BUKKIT)
+    env["test.$VERBOSE"]    = $VERBOSE
+    env["test.$LOAD_PATH"]  = $LOAD_PATH
+    env["test.Ping"]        = defined?(Ping)
+    @app.call(env)
+  end
+end
+
+use EnvMiddleware
+run TestRequest.new
diff --git a/test/spec_rackup.rb b/test/spec_rackup.rb
new file mode 100644
index 0000000..c3e46a7
--- /dev/null
+++ b/test/spec_rackup.rb
@@ -0,0 +1,133 @@
+require 'test/spec'
+require 'rack/rackup'
+require 'testrequest'
+require 'open3'
+
+begin
+require "mongrel"
+
+context "rackup" do
+  include TestRequest::Helpers
+
+  def run_rackup(*args)
+    options = args.last.is_a?(Hash) ? args.pop : {}
+    flags   = args.first
+    @host = options[:host] || "0.0.0.0"
+    @port = options[:port] || 9292
+
+    Dir.chdir("#{root}/test/rackup") do
+      @rackup = IO.popen("#{Gem.ruby} -S #{rackup} #{flags}")
+    end
+
+    return if options[:port] == false
+
+    # Wait until the server is available
+    begin
+      GET("/")
+    rescue
+      sleep 0.05
+      retry
+    end
+  end
+
+  def output
+    @rackup.read
+  end
+
+  after do
+    Process.kill(9, @rackup.pid) if @rackup
+
+    Dir["#{root}/**/*.pid"].each do |file|
+      Process.kill(9, File.read(file).to_i)
+      File.delete(file)
+    end
+  end
+
+  specify "rackup" do
+    run_rackup
+    response["PATH_INFO"].should.equal '/'
+    response["test.$DEBUG"].should.be false
+    response["test.$EVAL"].should.be nil
+    response["test.$VERBOSE"].should.be false
+    response["test.Ping"].should.be nil
+    response["SERVER_SOFTWARE"].should.not =~ /webrick/
+  end
+
+  specify "rackup --help" do
+    run_rackup "--help", :port => false
+    output.should.match /--port/
+  end
+
+  specify "rackup --port" do
+    run_rackup "--port 9000", :port => 9000
+    response["SERVER_PORT"].should.equal "9000"
+  end
+
+  specify "rackup --debug" do
+    run_rackup "--debug"
+    response["test.$DEBUG"].should.be true
+  end
+
+  specify "rackup --eval" do
+    run_rackup %{--eval "BUKKIT = 'BUKKIT'"}
+    response["test.$EVAL"].should.equal "BUKKIT"
+  end
+
+  specify "rackup --warn" do
+    run_rackup %{--warn}
+    response["test.$VERBOSE"].should.be true
+  end
+
+  specify "rackup --include" do
+    run_rackup %{--include /foo/bar}
+    response["test.$LOAD_PATH"].should.include "/foo/bar"
+  end
+
+  specify "rackup --require" do
+    run_rackup %{--require ping}
+    response["test.Ping"].should.equal "constant"
+  end
+
+  specify "rackup --server" do
+    run_rackup %{--server webrick}
+    response["SERVER_SOFTWARE"].should =~ /webrick/i
+  end
+
+  specify "rackup --host" do
+    run_rackup %{--host 127.0.0.1}, :host => "127.0.0.1"
+    response["REMOTE_ADDR"].should.equal "127.0.0.1"
+  end
+
+  specify "rackup --daemonize" do
+    run_rackup %{--daemonize}
+    status.should.be 200
+    @rackup.should.be.eof?
+  end
+
+  specify "rackup --pid" do
+    run_rackup %{--daemonize --pid testing.pid}
+    status.should.be 200
+    @rackup.should.be.eof?
+    Dir["#{root}/**/testing.pid"].should.not.be.empty?
+  end
+
+  specify "rackup --version" do
+    run_rackup %{--version}, :port => false
+    output.should =~ /1.0/
+  end
+
+  specify "rackup --env development includes lint" do
+    run_rackup
+    GET("/broken_lint")
+    status.should.be 500
+  end
+
+  specify "rackup --env" do
+    run_rackup %{--env deployment}
+    GET("/broken_lint")
+    status.should.be 200
+  end
+end
+rescue LoadError
+  $stderr.puts "Skipping rackup --server tests (mongrel is required). `gem install thin` and try again."
+end
\ No newline at end of file
diff --git a/test/testrequest.rb b/test/testrequest.rb
index 7b7190c..0da2b12 100644
--- a/test/testrequest.rb
+++ b/test/testrequest.rb
@@ -13,6 +13,17 @@ class TestRequest
   module Helpers
     attr_reader :status, :response
 
+    ROOT = File.expand_path(File.dirname(__FILE__) + "/..")
+    ENV["RUBYOPT"] = "-I#{ROOT}/lib -rubygems"
+
+    def root
+      ROOT
+    end
+
+    def rackup
+      "#{ROOT}/bin/rackup"
+    end
+
     def GET(path, header={})
       Net::HTTP.start(@host, @port) { |http|
         user = header.delete(:user)
@@ -22,7 +33,11 @@ class TestRequest
         get.basic_auth user, passwd  if user && passwd
         http.request(get) { |response|
           @status = response.code.to_i
-          @response = YAML.load(response.body)
+          begin
+            @response = YAML.load(response.body)
+          rescue ArgumentError
+            @response = nil
+          end
         }
       }
     end

[-- Attachment #3: rack_server_patch.diff --]
[-- Type: application/octet-stream, Size: 14908 bytes --]

diff --git a/bin/rackup b/bin/rackup
index 3a90327..cc9ccbf 100755
--- a/bin/rackup
+++ b/bin/rackup
@@ -1,176 +1,5 @@
 #!/usr/bin/env ruby
 # -*- ruby -*-
 
-require 'rack'
-
-require 'optparse'
-
-automatic = false
-server = nil
-env = "development"
-daemonize = false
-pid = File.expand_path("rack.pid")
-options = {:Port => 9292, :Host => "0.0.0.0", :AccessLog => []}
-
-# Don't evaluate CGI ISINDEX parameters.
-# http://hoohoo.ncsa.uiuc.edu/cgi/cl.html
-ARGV.clear  if ENV.include?("REQUEST_METHOD")
-
-opts = OptionParser.new("", 24, '  ') { |opts|
-  opts.banner = "Usage: rackup [ruby options] [rack options] [rackup config]"
-
-  opts.separator ""
-  opts.separator "Ruby options:"
-
-  lineno = 1
-  opts.on("-e", "--eval LINE", "evaluate a LINE of code") { |line|
-    eval line, TOPLEVEL_BINDING, "-e", lineno
-    lineno += 1
-  }
-
-  opts.on("-d", "--debug", "set debugging flags (set $DEBUG to true)") {
-    $DEBUG = true
-  }
-  opts.on("-w", "--warn", "turn warnings on for your script") {
-    $-w = true
-  }
-
-  opts.on("-I", "--include PATH",
-          "specify $LOAD_PATH (may be used more than once)") { |path|
-    $LOAD_PATH.unshift(*path.split(":"))
-  }
-
-  opts.on("-r", "--require LIBRARY",
-          "require the library, before executing your script") { |library|
-    require library
-  }
-
-  opts.separator ""
-  opts.separator "Rack options:"
-  opts.on("-s", "--server SERVER", "serve using SERVER (webrick/mongrel)") { |s|
-    server = s
-  }
-
-  opts.on("-o", "--host HOST", "listen on HOST (default: 0.0.0.0)") { |host|
-    options[:Host] = host
-  }
-
-  opts.on("-p", "--port PORT", "use PORT (default: 9292)") { |port|
-    options[:Port] = port
-  }
-
-  opts.on("-E", "--env ENVIRONMENT", "use ENVIRONMENT for defaults (default: development)") { |e|
-    env = e
-  }
-
-  opts.on("-D", "--daemonize", "run daemonized in the background") { |d|
-    daemonize = d ? true : false
-  }
-
-  opts.on("-P", "--pid FILE", "file to store PID (default: rack.pid)") { |f|
-    pid = File.expand_path(f)
-  }
-
-  opts.separator ""
-  opts.separator "Common options:"
-
-  opts.on_tail("-h", "--help", "Show this message") do
-    puts opts
-    exit
-  end
-
-  opts.on_tail("--version", "Show version") do
-    puts "Rack #{Rack.version}"
-    exit
-  end
-
-  opts.parse! ARGV
-}
-
-require 'pp'  if $DEBUG
-
-config = ARGV[0] || "config.ru"
-if !File.exist? config
-  abort "configuration #{config} not found"
-end
-
-if config =~ /\.ru$/
-  cfgfile = File.read(config)
-  if cfgfile[/^#\\(.*)/]
-    opts.parse! $1.split(/\s+/)
-  end
-  cfgfile.sub!(/^__END__\n.*/, '')
-  inner_app = eval "Rack::Builder.new {( " + cfgfile + "\n )}.to_app",
-                   nil, config
-else
-  require config
-  inner_app = Object.const_get(File.basename(config, '.rb').capitalize)
-end
-
-unless server = Rack::Handler.get(server)
-  # Guess.
-  if ENV.include?("PHP_FCGI_CHILDREN")
-    server = Rack::Handler::FastCGI
-
-    # We already speak FastCGI
-    options.delete :File
-    options.delete :Port
-  elsif ENV.include?("REQUEST_METHOD")
-    server = Rack::Handler::CGI
-  else
-    begin
-      server = Rack::Handler::Mongrel
-    rescue LoadError => e
-      server = Rack::Handler::WEBrick
-    end
-  end
-end
-
-p server  if $DEBUG
-
-case env
-when "development"
-  app = Rack::Builder.new {
-    use Rack::CommonLogger, $stderr  unless server.name =~ /CGI/
-    use Rack::ShowExceptions
-    use Rack::Lint
-    run inner_app
-  }.to_app
-
-when "deployment"
-  app = Rack::Builder.new {
-    use Rack::CommonLogger, $stderr  unless server.name =~ /CGI/
-    run inner_app
-  }.to_app
-
-when "none"
-  app = inner_app
-
-end
-
-if $DEBUG
-  pp app
-  pp inner_app
-end
-
-if daemonize
-  if RUBY_VERSION < "1.9"
-    exit if fork
-    Process.setsid
-    exit if fork
-    Dir.chdir "/"
-    File.umask 0000
-    STDIN.reopen "/dev/null"
-    STDOUT.reopen "/dev/null", "a"
-    STDERR.reopen "/dev/null", "a"
-  else
-    Process.daemon
-  end
-
-  if pid
-    File.open(pid, 'w'){ |f| f.write("#{Process.pid}") }
-    at_exit { File.delete(pid) if File.exist?(pid) }
-  end
-end
-
-server.run app, options
+require "rack"
+Rack::Server.start
\ No newline at end of file
diff --git a/lib/rack.rb b/lib/rack.rb
index 8d0815b..703649c 100644
--- a/lib/rack.rb
+++ b/lib/rack.rb
@@ -42,6 +42,7 @@ module Rack
   autoload :Mime, "rack/mime"
   autoload :Recursive, "rack/recursive"
   autoload :Reloader, "rack/reloader"
+  autoload :Server, "rack/server"
   autoload :ShowExceptions, "rack/showexceptions"
   autoload :ShowStatus, "rack/showstatus"
   autoload :Static, "rack/static"
diff --git a/lib/rack/builder.rb b/lib/rack/builder.rb
index 295235e..f769b5f 100644
--- a/lib/rack/builder.rb
+++ b/lib/rack/builder.rb
@@ -24,6 +24,21 @@ module Rack
   # You can use +map+ to construct a Rack::URLMap in a convenient way.
 
   class Builder
+    def self.parse_file(config, opts = nil)
+      if config =~ /\.ru$/
+        cfgfile = ::File.read(config)
+        if cfgfile[/^#\\(.*)/] && opts
+          opts.parse! $1.split(/\s+/)
+        end
+        cfgfile.sub!(/^__END__\n.*/, '')
+        eval "Rack::Builder.new {( " + cfgfile + "\n )}.to_app",
+          TOPLEVEL_BINDING, config
+      else
+        require config
+        Object.const_get(::File.basename(config, '.rb').capitalize)
+      end
+    end
+
     def initialize(&block)
       @ins = []
       instance_eval(&block) if block_given?
diff --git a/lib/rack/handler.rb b/lib/rack/handler.rb
index 5624a1e..3c09883 100644
--- a/lib/rack/handler.rb
+++ b/lib/rack/handler.rb
@@ -22,6 +22,25 @@ module Rack
       end
     end
 
+    def self.default(options = {})
+      # Guess.
+      if ENV.include?("PHP_FCGI_CHILDREN")
+        # We already speak FastCGI
+        options.delete :File
+        options.delete :Port
+
+        Rack::Handler::FastCGI
+      elsif ENV.include?("REQUEST_METHOD")
+        Rack::Handler::CGI
+      else
+        begin
+          Rack::Handler::Mongrel
+        rescue LoadError => e
+          Rack::Handler::WEBrick
+        end
+      end
+    end
+
     # Transforms server-name constants to their canonical form as filenames,
     # then tries to require them but silences the LoadError if not found
     #
diff --git a/lib/rack/server.rb b/lib/rack/server.rb
new file mode 100644
index 0000000..6130b28
--- /dev/null
+++ b/lib/rack/server.rb
@@ -0,0 +1,190 @@
+require 'optparse'
+
+module Rack
+  class Server
+    def self.start
+      new.start
+    end
+
+    attr_accessor :options
+
+    def initialize(options = nil)
+      @options = options
+    end
+
+    def options
+      @options ||= begin
+        parse_options(ARGV)
+      end
+    end
+
+    def default_options
+      {
+        :environment => "development",
+        :pid => ::File.expand_path("rack.pid"),
+        :Port => 9292,
+        :Host => "0.0.0.0",
+        :AccessLog => []
+      }
+    end
+
+    def app
+      @app ||= begin
+        if !::File.exist? options[:rack_file]
+          abort "configuration #{options[:rack_file]} not found"
+        end
+
+        Rack::Builder.parse_file(options[:rack_file], opt_parser)
+      end
+    end
+
+    def self.middleware
+      @middleware ||= begin
+        m = Hash.new {|h,k| h[k] = []}
+        m["deployment"].concat  [lambda {|server| server.server =~ /CGI/ ? nil : [Rack::CommonLogger, $stderr] }]
+        m["development"].concat m["deployment"] + [[Rack::ShowExceptions], [Rack::Lint]]
+        m
+      end
+    end
+
+    def middleware
+      self.class.middleware
+    end
+
+    def start
+      if $DEBUG
+        require 'pp'
+        p options[:server]
+        pp wrapped_app
+        pp app
+      end
+
+      daemonize_app if options[:daemonize]
+      write_pid if options[:pid]
+      server.run wrapped_app, options
+    end
+
+    def server
+      @_server ||= Rack::Handler.get(options[:server]) || Rack::Handler.default
+    end
+
+  private
+
+    def parse_options(args)
+      @options = default_options
+
+      # Don't evaluate CGI ISINDEX parameters.
+      # http://hoohoo.ncsa.uiuc.edu/cgi/cl.html
+      args.clear if ENV.include?("REQUEST_METHOD")
+
+      opt_parser.parse! args
+      @options[:rack_file] = args.last || ::File.expand_path("config.ru")
+      @options
+    end
+
+    def opt_parser
+      @opt_parser ||= OptionParser.new("", 24, '  ') do |opts|
+        opts.banner = "Usage: rackup [ruby options] [rack options] [rackup config]"
+
+        opts.separator ""
+        opts.separator "Ruby options:"
+
+        lineno = 1
+        opts.on("-e", "--eval LINE", "evaluate a LINE of code") { |line|
+          eval line, TOPLEVEL_BINDING, "-e", lineno
+          lineno += 1
+        }
+
+        opts.on("-d", "--debug", "set debugging flags (set $DEBUG to true)") {
+          $DEBUG = true
+        }
+        opts.on("-w", "--warn", "turn warnings on for your script") {
+          $-w = true
+        }
+
+        opts.on("-I", "--include PATH",
+                "specify $LOAD_PATH (may be used more than once)") { |path|
+          $LOAD_PATH.unshift(*path.split(":"))
+        }
+
+        opts.on("-r", "--require LIBRARY",
+                "require the library, before executing your script") { |library|
+          require library
+        }
+
+        opts.separator ""
+        opts.separator "Rack options:"
+        opts.on("-s", "--server SERVER", "serve using SERVER (webrick/mongrel)") { |s|
+          @options[:server] = s
+        }
+
+        opts.on("-o", "--host HOST", "listen on HOST (default: 0.0.0.0)") { |host|
+          @options[:Host] = host
+        }
+
+        opts.on("-p", "--port PORT", "use PORT (default: 9292)") { |port|
+          @options[:Port] = port
+        }
+
+        opts.on("-E", "--env ENVIRONMENT", "use ENVIRONMENT for defaults (default: development)") { |e|
+          @options[:environment] = e
+        }
+
+        opts.on("-D", "--daemonize", "run daemonized in the background") { |d|
+          @options[:daemonize] = d ? true : false
+        }
+
+        opts.on("-P", "--pid FILE", "file to store PID (default: rack.pid)") { |f|
+          @options[:pid] = ::File.expand_path(f)
+        }
+
+        opts.separator ""
+        opts.separator "Common options:"
+
+        opts.on_tail("-h", "--help", "Show this message") do
+          puts opts
+          exit
+        end
+
+        opts.on_tail("--version", "Show version") do
+          puts "Rack #{Rack.version}"
+          exit
+        end
+      end
+    end
+
+    def build_app(app)
+      middleware[options[:environment]].reverse_each do |middleware|
+        middleware = middleware.call(self) if middleware.respond_to?(:call)
+        next unless middleware
+        klass = middleware.shift
+        app = klass.new(app, *middleware)
+      end
+      app
+    end
+
+    def wrapped_app
+      @wrapped_app ||= build_app app
+    end
+
+    def daemonize_app
+      if RUBY_VERSION < "1.9"
+        exit if fork
+        Process.setsid
+        exit if fork
+        Dir.chdir "/"
+        ::File.umask 0000
+        STDIN.reopen "/dev/null"
+        STDOUT.reopen "/dev/null", "a"
+        STDERR.reopen "/dev/null", "a"
+      else
+        Process.daemon
+      end
+    end
+
+    def write_pid
+      ::File.open(options[:pid], 'w'){ |f| f.write("#{Process.pid}") }
+      at_exit { ::File.delete(options[:pid]) if ::File.exist?(options[:pid]) }
+    end
+  end
+end
\ No newline at end of file
diff --git a/test/rackup/config.ru b/test/rackup/config.ru
index 2490c6e..3ca5308 100644
--- a/test/rackup/config.ru
+++ b/test/rackup/config.ru
@@ -1,6 +1,6 @@
 require "#{File.dirname(__FILE__)}/../testrequest"
 
-$stderr = StringIO.new
+$stderr = File.open("#{File.dirname(__FILE__)}/log_output", "w")
 
 class EnvMiddleware
   def initialize(app)
@@ -8,15 +8,21 @@ class EnvMiddleware
   end
   
   def call(env)
+    # provides a way to test that lint is present
     if env["PATH_INFO"] == "/broken_lint"
       return [200, {}, ["Broken Lint"]]
+    # provides a way to kill the process without knowing the pid
+    elsif env["PATH_INFO"] == "/die"
+      exit!
     end
 
     env["test.$DEBUG"]      = $DEBUG
     env["test.$EVAL"]       = BUKKIT if defined?(BUKKIT)
     env["test.$VERBOSE"]    = $VERBOSE
     env["test.$LOAD_PATH"]  = $LOAD_PATH
+    env["test.stderr"]      = File.expand_path($stderr.path)
     env["test.Ping"]        = defined?(Ping)
+    env["test.pid"]         = Process.pid
     @app.call(env)
   end
 end
diff --git a/test/spec_rackup.rb b/test/spec_rackup.rb
index c3e46a7..1c0a00d 100644
--- a/test/spec_rackup.rb
+++ b/test/spec_rackup.rb
@@ -1,5 +1,5 @@
 require 'test/spec'
-require 'rack/rackup'
+require 'rack/server'
 require 'testrequest'
 require 'open3'
 
@@ -16,7 +16,7 @@ context "rackup" do
     @port = options[:port] || 9292
 
     Dir.chdir("#{root}/test/rackup") do
-      @rackup = IO.popen("#{Gem.ruby} -S #{rackup} #{flags}")
+      @in, @rackup, @err = Open3.popen3("#{Gem.ruby} -S #{rackup} #{flags}")
     end
 
     return if options[:port] == false
@@ -35,12 +35,14 @@ context "rackup" do
   end
 
   after do
-    Process.kill(9, @rackup.pid) if @rackup
+    # This doesn't actually return a response, so we rescue
+    GET "/die" rescue nil
 
     Dir["#{root}/**/*.pid"].each do |file|
-      Process.kill(9, File.read(file).to_i)
       File.delete(file)
     end
+
+    File.delete("#{root}/log_output") if File.exist?("#{root}/log_output")
   end
 
   specify "rackup" do
@@ -104,10 +106,15 @@ context "rackup" do
     @rackup.should.be.eof?
   end
 
-  specify "rackup --pid" do
+  specify "rackup --daemonize --pid" do
     run_rackup %{--daemonize --pid testing.pid}
-    status.should.be 200
     @rackup.should.be.eof?
+    File.read("#{root}/test/rackup/testing.pid").should.equal response["test.pid"].to_s
+  end
+
+  specify "rackup --pid" do
+    run_rackup %{--pid testing.pid}
+    status.should.be 200
     Dir["#{root}/**/testing.pid"].should.not.be.empty?
   end
 
@@ -122,11 +129,30 @@ context "rackup" do
     status.should.be 500
   end
 
-  specify "rackup --env" do
+  specify "rackup --env deployment does not include lint" do
     run_rackup %{--env deployment}
     GET("/broken_lint")
     status.should.be 200
   end
+
+  specify "rackup --env none does not include lint" do
+    run_rackup %{--env none}
+    GET("/broken_lint")
+    status.should.be 200
+  end
+
+  specify "rackup --env deployment does log" do
+    run_rackup %{--env deployment}
+    log = File.read(response["test.stderr"])
+    log.should.be.empty?
+  end
+
+  specify "rackup --env none does not log" do
+    run_rackup %{--env none}
+    GET("/")
+    log = File.read(response["test.stderr"])
+    log.should.be.empty?
+  end
 end
 rescue LoadError
   $stderr.puts "Skipping rackup --server tests (mongrel is required). `gem install thin` and try again."

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Rack::Server patch
  2009-11-21  0:42 Rack::Server patch Yehuda Katz
  2009-11-21  0:46 ` Yehuda Katz
@ 2009-11-21  0:52 ` Yehuda Katz
  2009-11-21  2:10   ` Joshua Peek
  2009-11-21 10:18   ` Christian Neukirchen
  1 sibling, 2 replies; 6+ messages in thread
From: Yehuda Katz @ 2009-11-21  0:52 UTC (permalink / raw)
  To: rack-devel


[-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --]

Patches attached.

[1] tests_patch.diff -- Adds tests for the functionality of rackup
[2] rack_server_patch.diff -- Moves Rack::Server into a separate object

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325


2009/11/20 Yehuda Katz <wycats@gmail.com>

> Hey guys,
>
> Carl and I spent a couple of day refactoring the code in bin/rackup into
> Rack::Server. The main motivation for this was to enable Rails to ditch our
> code in script/server and simply inherit from the requisite Rack code. I
> think the code improvement speaks for itself. In the process of this work,
> we also moved a few things out of Rackup into more usable locations, like
> Rack::Handler.default (to get the handler that Rack will use if none is
> specified) and Rack::Builder.parse_file (which we currently duplicate in
> ActionDispatch).
>
> You can check out the changes at github.com/carllerche/rack, and I have
> also attached a patch. In addition to converting the rackup binary to a
> class, we also wrote tests for each function of rackup, to be sure we
> wouldn't break anything in the refactor. As a result, this patch now has
> tests for rackup!
>
> Yehuda Katz
> Developer | Engine Yard
> (ph) 718.877.1325
>

[-- Attachment #1.2: Type: text/html, Size: 1635 bytes --]

[-- Attachment #2: tests_patch.diff --]
[-- Type: application/octet-stream, Size: 5366 bytes --]

diff --git a/bin/rackup b/bin/rackup
index 91abe17..3a90327 100755
--- a/bin/rackup
+++ b/bin/rackup
@@ -9,7 +9,7 @@ automatic = false
 server = nil
 env = "development"
 daemonize = false
-pid = nil
+pid = File.expand_path("rack.pid")
 options = {:Port => 9292, :Host => "0.0.0.0", :AccessLog => []}
 
 # Don't evaluate CGI ISINDEX parameters.
diff --git a/test/rackup/config.ru b/test/rackup/config.ru
new file mode 100644
index 0000000..2490c6e
--- /dev/null
+++ b/test/rackup/config.ru
@@ -0,0 +1,25 @@
+require "#{File.dirname(__FILE__)}/../testrequest"
+
+$stderr = StringIO.new
+
+class EnvMiddleware
+  def initialize(app)
+    @app = app
+  end
+  
+  def call(env)
+    if env["PATH_INFO"] == "/broken_lint"
+      return [200, {}, ["Broken Lint"]]
+    end
+
+    env["test.$DEBUG"]      = $DEBUG
+    env["test.$EVAL"]       = BUKKIT if defined?(BUKKIT)
+    env["test.$VERBOSE"]    = $VERBOSE
+    env["test.$LOAD_PATH"]  = $LOAD_PATH
+    env["test.Ping"]        = defined?(Ping)
+    @app.call(env)
+  end
+end
+
+use EnvMiddleware
+run TestRequest.new
diff --git a/test/spec_rackup.rb b/test/spec_rackup.rb
new file mode 100644
index 0000000..c3e46a7
--- /dev/null
+++ b/test/spec_rackup.rb
@@ -0,0 +1,133 @@
+require 'test/spec'
+require 'rack/rackup'
+require 'testrequest'
+require 'open3'
+
+begin
+require "mongrel"
+
+context "rackup" do
+  include TestRequest::Helpers
+
+  def run_rackup(*args)
+    options = args.last.is_a?(Hash) ? args.pop : {}
+    flags   = args.first
+    @host = options[:host] || "0.0.0.0"
+    @port = options[:port] || 9292
+
+    Dir.chdir("#{root}/test/rackup") do
+      @rackup = IO.popen("#{Gem.ruby} -S #{rackup} #{flags}")
+    end
+
+    return if options[:port] == false
+
+    # Wait until the server is available
+    begin
+      GET("/")
+    rescue
+      sleep 0.05
+      retry
+    end
+  end
+
+  def output
+    @rackup.read
+  end
+
+  after do
+    Process.kill(9, @rackup.pid) if @rackup
+
+    Dir["#{root}/**/*.pid"].each do |file|
+      Process.kill(9, File.read(file).to_i)
+      File.delete(file)
+    end
+  end
+
+  specify "rackup" do
+    run_rackup
+    response["PATH_INFO"].should.equal '/'
+    response["test.$DEBUG"].should.be false
+    response["test.$EVAL"].should.be nil
+    response["test.$VERBOSE"].should.be false
+    response["test.Ping"].should.be nil
+    response["SERVER_SOFTWARE"].should.not =~ /webrick/
+  end
+
+  specify "rackup --help" do
+    run_rackup "--help", :port => false
+    output.should.match /--port/
+  end
+
+  specify "rackup --port" do
+    run_rackup "--port 9000", :port => 9000
+    response["SERVER_PORT"].should.equal "9000"
+  end
+
+  specify "rackup --debug" do
+    run_rackup "--debug"
+    response["test.$DEBUG"].should.be true
+  end
+
+  specify "rackup --eval" do
+    run_rackup %{--eval "BUKKIT = 'BUKKIT'"}
+    response["test.$EVAL"].should.equal "BUKKIT"
+  end
+
+  specify "rackup --warn" do
+    run_rackup %{--warn}
+    response["test.$VERBOSE"].should.be true
+  end
+
+  specify "rackup --include" do
+    run_rackup %{--include /foo/bar}
+    response["test.$LOAD_PATH"].should.include "/foo/bar"
+  end
+
+  specify "rackup --require" do
+    run_rackup %{--require ping}
+    response["test.Ping"].should.equal "constant"
+  end
+
+  specify "rackup --server" do
+    run_rackup %{--server webrick}
+    response["SERVER_SOFTWARE"].should =~ /webrick/i
+  end
+
+  specify "rackup --host" do
+    run_rackup %{--host 127.0.0.1}, :host => "127.0.0.1"
+    response["REMOTE_ADDR"].should.equal "127.0.0.1"
+  end
+
+  specify "rackup --daemonize" do
+    run_rackup %{--daemonize}
+    status.should.be 200
+    @rackup.should.be.eof?
+  end
+
+  specify "rackup --pid" do
+    run_rackup %{--daemonize --pid testing.pid}
+    status.should.be 200
+    @rackup.should.be.eof?
+    Dir["#{root}/**/testing.pid"].should.not.be.empty?
+  end
+
+  specify "rackup --version" do
+    run_rackup %{--version}, :port => false
+    output.should =~ /1.0/
+  end
+
+  specify "rackup --env development includes lint" do
+    run_rackup
+    GET("/broken_lint")
+    status.should.be 500
+  end
+
+  specify "rackup --env" do
+    run_rackup %{--env deployment}
+    GET("/broken_lint")
+    status.should.be 200
+  end
+end
+rescue LoadError
+  $stderr.puts "Skipping rackup --server tests (mongrel is required). `gem install thin` and try again."
+end
\ No newline at end of file
diff --git a/test/testrequest.rb b/test/testrequest.rb
index 7b7190c..0da2b12 100644
--- a/test/testrequest.rb
+++ b/test/testrequest.rb
@@ -13,6 +13,17 @@ class TestRequest
   module Helpers
     attr_reader :status, :response
 
+    ROOT = File.expand_path(File.dirname(__FILE__) + "/..")
+    ENV["RUBYOPT"] = "-I#{ROOT}/lib -rubygems"
+
+    def root
+      ROOT
+    end
+
+    def rackup
+      "#{ROOT}/bin/rackup"
+    end
+
     def GET(path, header={})
       Net::HTTP.start(@host, @port) { |http|
         user = header.delete(:user)
@@ -22,7 +33,11 @@ class TestRequest
         get.basic_auth user, passwd  if user && passwd
         http.request(get) { |response|
           @status = response.code.to_i
-          @response = YAML.load(response.body)
+          begin
+            @response = YAML.load(response.body)
+          rescue ArgumentError
+            @response = nil
+          end
         }
       }
     end

[-- Attachment #3: rack_server_patch.diff --]
[-- Type: application/octet-stream, Size: 14908 bytes --]

diff --git a/bin/rackup b/bin/rackup
index 3a90327..cc9ccbf 100755
--- a/bin/rackup
+++ b/bin/rackup
@@ -1,176 +1,5 @@
 #!/usr/bin/env ruby
 # -*- ruby -*-
 
-require 'rack'
-
-require 'optparse'
-
-automatic = false
-server = nil
-env = "development"
-daemonize = false
-pid = File.expand_path("rack.pid")
-options = {:Port => 9292, :Host => "0.0.0.0", :AccessLog => []}
-
-# Don't evaluate CGI ISINDEX parameters.
-# http://hoohoo.ncsa.uiuc.edu/cgi/cl.html
-ARGV.clear  if ENV.include?("REQUEST_METHOD")
-
-opts = OptionParser.new("", 24, '  ') { |opts|
-  opts.banner = "Usage: rackup [ruby options] [rack options] [rackup config]"
-
-  opts.separator ""
-  opts.separator "Ruby options:"
-
-  lineno = 1
-  opts.on("-e", "--eval LINE", "evaluate a LINE of code") { |line|
-    eval line, TOPLEVEL_BINDING, "-e", lineno
-    lineno += 1
-  }
-
-  opts.on("-d", "--debug", "set debugging flags (set $DEBUG to true)") {
-    $DEBUG = true
-  }
-  opts.on("-w", "--warn", "turn warnings on for your script") {
-    $-w = true
-  }
-
-  opts.on("-I", "--include PATH",
-          "specify $LOAD_PATH (may be used more than once)") { |path|
-    $LOAD_PATH.unshift(*path.split(":"))
-  }
-
-  opts.on("-r", "--require LIBRARY",
-          "require the library, before executing your script") { |library|
-    require library
-  }
-
-  opts.separator ""
-  opts.separator "Rack options:"
-  opts.on("-s", "--server SERVER", "serve using SERVER (webrick/mongrel)") { |s|
-    server = s
-  }
-
-  opts.on("-o", "--host HOST", "listen on HOST (default: 0.0.0.0)") { |host|
-    options[:Host] = host
-  }
-
-  opts.on("-p", "--port PORT", "use PORT (default: 9292)") { |port|
-    options[:Port] = port
-  }
-
-  opts.on("-E", "--env ENVIRONMENT", "use ENVIRONMENT for defaults (default: development)") { |e|
-    env = e
-  }
-
-  opts.on("-D", "--daemonize", "run daemonized in the background") { |d|
-    daemonize = d ? true : false
-  }
-
-  opts.on("-P", "--pid FILE", "file to store PID (default: rack.pid)") { |f|
-    pid = File.expand_path(f)
-  }
-
-  opts.separator ""
-  opts.separator "Common options:"
-
-  opts.on_tail("-h", "--help", "Show this message") do
-    puts opts
-    exit
-  end
-
-  opts.on_tail("--version", "Show version") do
-    puts "Rack #{Rack.version}"
-    exit
-  end
-
-  opts.parse! ARGV
-}
-
-require 'pp'  if $DEBUG
-
-config = ARGV[0] || "config.ru"
-if !File.exist? config
-  abort "configuration #{config} not found"
-end
-
-if config =~ /\.ru$/
-  cfgfile = File.read(config)
-  if cfgfile[/^#\\(.*)/]
-    opts.parse! $1.split(/\s+/)
-  end
-  cfgfile.sub!(/^__END__\n.*/, '')
-  inner_app = eval "Rack::Builder.new {( " + cfgfile + "\n )}.to_app",
-                   nil, config
-else
-  require config
-  inner_app = Object.const_get(File.basename(config, '.rb').capitalize)
-end
-
-unless server = Rack::Handler.get(server)
-  # Guess.
-  if ENV.include?("PHP_FCGI_CHILDREN")
-    server = Rack::Handler::FastCGI
-
-    # We already speak FastCGI
-    options.delete :File
-    options.delete :Port
-  elsif ENV.include?("REQUEST_METHOD")
-    server = Rack::Handler::CGI
-  else
-    begin
-      server = Rack::Handler::Mongrel
-    rescue LoadError => e
-      server = Rack::Handler::WEBrick
-    end
-  end
-end
-
-p server  if $DEBUG
-
-case env
-when "development"
-  app = Rack::Builder.new {
-    use Rack::CommonLogger, $stderr  unless server.name =~ /CGI/
-    use Rack::ShowExceptions
-    use Rack::Lint
-    run inner_app
-  }.to_app
-
-when "deployment"
-  app = Rack::Builder.new {
-    use Rack::CommonLogger, $stderr  unless server.name =~ /CGI/
-    run inner_app
-  }.to_app
-
-when "none"
-  app = inner_app
-
-end
-
-if $DEBUG
-  pp app
-  pp inner_app
-end
-
-if daemonize
-  if RUBY_VERSION < "1.9"
-    exit if fork
-    Process.setsid
-    exit if fork
-    Dir.chdir "/"
-    File.umask 0000
-    STDIN.reopen "/dev/null"
-    STDOUT.reopen "/dev/null", "a"
-    STDERR.reopen "/dev/null", "a"
-  else
-    Process.daemon
-  end
-
-  if pid
-    File.open(pid, 'w'){ |f| f.write("#{Process.pid}") }
-    at_exit { File.delete(pid) if File.exist?(pid) }
-  end
-end
-
-server.run app, options
+require "rack"
+Rack::Server.start
\ No newline at end of file
diff --git a/lib/rack.rb b/lib/rack.rb
index 8d0815b..703649c 100644
--- a/lib/rack.rb
+++ b/lib/rack.rb
@@ -42,6 +42,7 @@ module Rack
   autoload :Mime, "rack/mime"
   autoload :Recursive, "rack/recursive"
   autoload :Reloader, "rack/reloader"
+  autoload :Server, "rack/server"
   autoload :ShowExceptions, "rack/showexceptions"
   autoload :ShowStatus, "rack/showstatus"
   autoload :Static, "rack/static"
diff --git a/lib/rack/builder.rb b/lib/rack/builder.rb
index 295235e..f769b5f 100644
--- a/lib/rack/builder.rb
+++ b/lib/rack/builder.rb
@@ -24,6 +24,21 @@ module Rack
   # You can use +map+ to construct a Rack::URLMap in a convenient way.
 
   class Builder
+    def self.parse_file(config, opts = nil)
+      if config =~ /\.ru$/
+        cfgfile = ::File.read(config)
+        if cfgfile[/^#\\(.*)/] && opts
+          opts.parse! $1.split(/\s+/)
+        end
+        cfgfile.sub!(/^__END__\n.*/, '')
+        eval "Rack::Builder.new {( " + cfgfile + "\n )}.to_app",
+          TOPLEVEL_BINDING, config
+      else
+        require config
+        Object.const_get(::File.basename(config, '.rb').capitalize)
+      end
+    end
+
     def initialize(&block)
       @ins = []
       instance_eval(&block) if block_given?
diff --git a/lib/rack/handler.rb b/lib/rack/handler.rb
index 5624a1e..3c09883 100644
--- a/lib/rack/handler.rb
+++ b/lib/rack/handler.rb
@@ -22,6 +22,25 @@ module Rack
       end
     end
 
+    def self.default(options = {})
+      # Guess.
+      if ENV.include?("PHP_FCGI_CHILDREN")
+        # We already speak FastCGI
+        options.delete :File
+        options.delete :Port
+
+        Rack::Handler::FastCGI
+      elsif ENV.include?("REQUEST_METHOD")
+        Rack::Handler::CGI
+      else
+        begin
+          Rack::Handler::Mongrel
+        rescue LoadError => e
+          Rack::Handler::WEBrick
+        end
+      end
+    end
+
     # Transforms server-name constants to their canonical form as filenames,
     # then tries to require them but silences the LoadError if not found
     #
diff --git a/lib/rack/server.rb b/lib/rack/server.rb
new file mode 100644
index 0000000..6130b28
--- /dev/null
+++ b/lib/rack/server.rb
@@ -0,0 +1,190 @@
+require 'optparse'
+
+module Rack
+  class Server
+    def self.start
+      new.start
+    end
+
+    attr_accessor :options
+
+    def initialize(options = nil)
+      @options = options
+    end
+
+    def options
+      @options ||= begin
+        parse_options(ARGV)
+      end
+    end
+
+    def default_options
+      {
+        :environment => "development",
+        :pid => ::File.expand_path("rack.pid"),
+        :Port => 9292,
+        :Host => "0.0.0.0",
+        :AccessLog => []
+      }
+    end
+
+    def app
+      @app ||= begin
+        if !::File.exist? options[:rack_file]
+          abort "configuration #{options[:rack_file]} not found"
+        end
+
+        Rack::Builder.parse_file(options[:rack_file], opt_parser)
+      end
+    end
+
+    def self.middleware
+      @middleware ||= begin
+        m = Hash.new {|h,k| h[k] = []}
+        m["deployment"].concat  [lambda {|server| server.server =~ /CGI/ ? nil : [Rack::CommonLogger, $stderr] }]
+        m["development"].concat m["deployment"] + [[Rack::ShowExceptions], [Rack::Lint]]
+        m
+      end
+    end
+
+    def middleware
+      self.class.middleware
+    end
+
+    def start
+      if $DEBUG
+        require 'pp'
+        p options[:server]
+        pp wrapped_app
+        pp app
+      end
+
+      daemonize_app if options[:daemonize]
+      write_pid if options[:pid]
+      server.run wrapped_app, options
+    end
+
+    def server
+      @_server ||= Rack::Handler.get(options[:server]) || Rack::Handler.default
+    end
+
+  private
+
+    def parse_options(args)
+      @options = default_options
+
+      # Don't evaluate CGI ISINDEX parameters.
+      # http://hoohoo.ncsa.uiuc.edu/cgi/cl.html
+      args.clear if ENV.include?("REQUEST_METHOD")
+
+      opt_parser.parse! args
+      @options[:rack_file] = args.last || ::File.expand_path("config.ru")
+      @options
+    end
+
+    def opt_parser
+      @opt_parser ||= OptionParser.new("", 24, '  ') do |opts|
+        opts.banner = "Usage: rackup [ruby options] [rack options] [rackup config]"
+
+        opts.separator ""
+        opts.separator "Ruby options:"
+
+        lineno = 1
+        opts.on("-e", "--eval LINE", "evaluate a LINE of code") { |line|
+          eval line, TOPLEVEL_BINDING, "-e", lineno
+          lineno += 1
+        }
+
+        opts.on("-d", "--debug", "set debugging flags (set $DEBUG to true)") {
+          $DEBUG = true
+        }
+        opts.on("-w", "--warn", "turn warnings on for your script") {
+          $-w = true
+        }
+
+        opts.on("-I", "--include PATH",
+                "specify $LOAD_PATH (may be used more than once)") { |path|
+          $LOAD_PATH.unshift(*path.split(":"))
+        }
+
+        opts.on("-r", "--require LIBRARY",
+                "require the library, before executing your script") { |library|
+          require library
+        }
+
+        opts.separator ""
+        opts.separator "Rack options:"
+        opts.on("-s", "--server SERVER", "serve using SERVER (webrick/mongrel)") { |s|
+          @options[:server] = s
+        }
+
+        opts.on("-o", "--host HOST", "listen on HOST (default: 0.0.0.0)") { |host|
+          @options[:Host] = host
+        }
+
+        opts.on("-p", "--port PORT", "use PORT (default: 9292)") { |port|
+          @options[:Port] = port
+        }
+
+        opts.on("-E", "--env ENVIRONMENT", "use ENVIRONMENT for defaults (default: development)") { |e|
+          @options[:environment] = e
+        }
+
+        opts.on("-D", "--daemonize", "run daemonized in the background") { |d|
+          @options[:daemonize] = d ? true : false
+        }
+
+        opts.on("-P", "--pid FILE", "file to store PID (default: rack.pid)") { |f|
+          @options[:pid] = ::File.expand_path(f)
+        }
+
+        opts.separator ""
+        opts.separator "Common options:"
+
+        opts.on_tail("-h", "--help", "Show this message") do
+          puts opts
+          exit
+        end
+
+        opts.on_tail("--version", "Show version") do
+          puts "Rack #{Rack.version}"
+          exit
+        end
+      end
+    end
+
+    def build_app(app)
+      middleware[options[:environment]].reverse_each do |middleware|
+        middleware = middleware.call(self) if middleware.respond_to?(:call)
+        next unless middleware
+        klass = middleware.shift
+        app = klass.new(app, *middleware)
+      end
+      app
+    end
+
+    def wrapped_app
+      @wrapped_app ||= build_app app
+    end
+
+    def daemonize_app
+      if RUBY_VERSION < "1.9"
+        exit if fork
+        Process.setsid
+        exit if fork
+        Dir.chdir "/"
+        ::File.umask 0000
+        STDIN.reopen "/dev/null"
+        STDOUT.reopen "/dev/null", "a"
+        STDERR.reopen "/dev/null", "a"
+      else
+        Process.daemon
+      end
+    end
+
+    def write_pid
+      ::File.open(options[:pid], 'w'){ |f| f.write("#{Process.pid}") }
+      at_exit { ::File.delete(options[:pid]) if ::File.exist?(options[:pid]) }
+    end
+  end
+end
\ No newline at end of file
diff --git a/test/rackup/config.ru b/test/rackup/config.ru
index 2490c6e..3ca5308 100644
--- a/test/rackup/config.ru
+++ b/test/rackup/config.ru
@@ -1,6 +1,6 @@
 require "#{File.dirname(__FILE__)}/../testrequest"
 
-$stderr = StringIO.new
+$stderr = File.open("#{File.dirname(__FILE__)}/log_output", "w")
 
 class EnvMiddleware
   def initialize(app)
@@ -8,15 +8,21 @@ class EnvMiddleware
   end
   
   def call(env)
+    # provides a way to test that lint is present
     if env["PATH_INFO"] == "/broken_lint"
       return [200, {}, ["Broken Lint"]]
+    # provides a way to kill the process without knowing the pid
+    elsif env["PATH_INFO"] == "/die"
+      exit!
     end
 
     env["test.$DEBUG"]      = $DEBUG
     env["test.$EVAL"]       = BUKKIT if defined?(BUKKIT)
     env["test.$VERBOSE"]    = $VERBOSE
     env["test.$LOAD_PATH"]  = $LOAD_PATH
+    env["test.stderr"]      = File.expand_path($stderr.path)
     env["test.Ping"]        = defined?(Ping)
+    env["test.pid"]         = Process.pid
     @app.call(env)
   end
 end
diff --git a/test/spec_rackup.rb b/test/spec_rackup.rb
index c3e46a7..1c0a00d 100644
--- a/test/spec_rackup.rb
+++ b/test/spec_rackup.rb
@@ -1,5 +1,5 @@
 require 'test/spec'
-require 'rack/rackup'
+require 'rack/server'
 require 'testrequest'
 require 'open3'
 
@@ -16,7 +16,7 @@ context "rackup" do
     @port = options[:port] || 9292
 
     Dir.chdir("#{root}/test/rackup") do
-      @rackup = IO.popen("#{Gem.ruby} -S #{rackup} #{flags}")
+      @in, @rackup, @err = Open3.popen3("#{Gem.ruby} -S #{rackup} #{flags}")
     end
 
     return if options[:port] == false
@@ -35,12 +35,14 @@ context "rackup" do
   end
 
   after do
-    Process.kill(9, @rackup.pid) if @rackup
+    # This doesn't actually return a response, so we rescue
+    GET "/die" rescue nil
 
     Dir["#{root}/**/*.pid"].each do |file|
-      Process.kill(9, File.read(file).to_i)
       File.delete(file)
     end
+
+    File.delete("#{root}/log_output") if File.exist?("#{root}/log_output")
   end
 
   specify "rackup" do
@@ -104,10 +106,15 @@ context "rackup" do
     @rackup.should.be.eof?
   end
 
-  specify "rackup --pid" do
+  specify "rackup --daemonize --pid" do
     run_rackup %{--daemonize --pid testing.pid}
-    status.should.be 200
     @rackup.should.be.eof?
+    File.read("#{root}/test/rackup/testing.pid").should.equal response["test.pid"].to_s
+  end
+
+  specify "rackup --pid" do
+    run_rackup %{--pid testing.pid}
+    status.should.be 200
     Dir["#{root}/**/testing.pid"].should.not.be.empty?
   end
 
@@ -122,11 +129,30 @@ context "rackup" do
     status.should.be 500
   end
 
-  specify "rackup --env" do
+  specify "rackup --env deployment does not include lint" do
     run_rackup %{--env deployment}
     GET("/broken_lint")
     status.should.be 200
   end
+
+  specify "rackup --env none does not include lint" do
+    run_rackup %{--env none}
+    GET("/broken_lint")
+    status.should.be 200
+  end
+
+  specify "rackup --env deployment does log" do
+    run_rackup %{--env deployment}
+    log = File.read(response["test.stderr"])
+    log.should.be.empty?
+  end
+
+  specify "rackup --env none does not log" do
+    run_rackup %{--env none}
+    GET("/")
+    log = File.read(response["test.stderr"])
+    log.should.be.empty?
+  end
 end
 rescue LoadError
   $stderr.puts "Skipping rackup --server tests (mongrel is required). `gem install thin` and try again."

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Rack::Server patch
  2009-11-21  0:52 ` Yehuda Katz
@ 2009-11-21  2:10   ` Joshua Peek
  2009-11-21 10:17     ` Christian Neukirchen
  2009-11-21 10:18   ` Christian Neukirchen
  1 sibling, 1 reply; 6+ messages in thread
From: Joshua Peek @ 2009-11-21  2:10 UTC (permalink / raw)
  To: rack-devel

+1 Looks good.

My only suggestion would be to keep the old pid/daemonize behavior. So
don't set a default pid file and only set one if its provided.

On Fri, Nov 20, 2009 at 4:52 PM, Yehuda Katz <wycats@gmail.com> wrote:
> Patches attached.
> [1] tests_patch.diff -- Adds tests for the functionality of rackup
> [2] rack_server_patch.diff -- Moves Rack::Server into a separate object
> Yehuda Katz
> Developer | Engine Yard
> (ph) 718.877.1325
>
>
> 2009/11/20 Yehuda Katz <wycats@gmail.com>
>>
>> Hey guys,
>> Carl and I spent a couple of day refactoring the code in bin/rackup into
>> Rack::Server. The main motivation for this was to enable Rails to ditch our
>> code in script/server and simply inherit from the requisite Rack code. I
>> think the code improvement speaks for itself. In the process of this work,
>> we also moved a few things out of Rackup into more usable locations, like
>> Rack::Handler.default (to get the handler that Rack will use if none is
>> specified) and Rack::Builder.parse_file (which we currently duplicate in
>> ActionDispatch).
>> You can check out the changes at github.com/carllerche/rack, and I have
>> also attached a patch. In addition to converting the rackup binary to a
>> class, we also wrote tests for each function of rackup, to be sure we
>> wouldn't break anything in the refactor. As a result, this patch now has
>> tests for rackup!
>> Yehuda Katz
>> Developer | Engine Yard
>> (ph) 718.877.1325
>
>



-- 
Joshua Peek

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Rack::Server patch
  2009-11-21  2:10   ` Joshua Peek
@ 2009-11-21 10:17     ` Christian Neukirchen
  0 siblings, 0 replies; 6+ messages in thread
From: Christian Neukirchen @ 2009-11-21 10:17 UTC (permalink / raw)
  To: rack-devel

Joshua Peek <josh@joshpeek.com> writes:

> So
> don't set a default pid file and only set one if its provided.

+1

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Rack::Server patch
  2009-11-21  0:52 ` Yehuda Katz
  2009-11-21  2:10   ` Joshua Peek
@ 2009-11-21 10:18   ` Christian Neukirchen
  1 sibling, 0 replies; 6+ messages in thread
From: Christian Neukirchen @ 2009-11-21 10:18 UTC (permalink / raw)
  To: rack-devel

Yehuda Katz <wycats@gmail.com> writes:

> [1] tests_patch.diff -- Adds tests for the functionality of rackup
> [2] rack_server_patch.diff -- Moves Rack::Server into a separate object

LGTM.

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2009-11-21 10:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-21  0:42 Rack::Server patch Yehuda Katz
2009-11-21  0:46 ` Yehuda Katz
2009-11-21  0:52 ` Yehuda Katz
2009-11-21  2:10   ` Joshua Peek
2009-11-21 10:17     ` Christian Neukirchen
2009-11-21 10:18   ` Christian Neukirchen

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/rack.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).