Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix random timeouts during Server#boot #920

Closed
wants to merge 1 commit into from

4 participants

@jdelStrother

Rack::Handler::Thin.run() (and, by extension, Capybara.server.call())
can return immediately if another Thin handler is already running. This
results in a race condition where @server_thread can finish running
before we evaluate responsive?. The easiest solution seems to be to
just remove the check that @server_thread is alive - it's quite possible
that EventMachine has merged the new server into a previous one, so we don't
particularly care about the state of our own thread.


I was having trouble running Capybara's tests successfully - I would keep getting timeout errors while booting, particularly in current_url_spec.rb and server_spec.rb (both of which create multiple Capybara::Servers), and eventually traced it down to this. Has anyone else had trouble with that or is it just me?

@jnicklas
Owner

I have no idea why we are doing that check anyway. @joliss, any ideas?

@joliss
Collaborator

I'm not sure. It was added by @raggi in ab62b27. @raggi, any insights?

@joliss
Collaborator

I was having trouble running Capybara's tests successfully - I would keep getting timeout errors while booting, particularly in current_url_spec.rb and server_spec.rb (both of which create multiple Capybara::Servers), and eventually traced it down to this. Has anyone else had trouble with that or is it just me?

I haven't seen it, but it's quite possible that we're having a heisenbug there. Could you post a stack trace for posterity?

@jdelStrother

It's just a timeout from the Timeout block not completing, but it looks something like :

     Timeout::Error:
       execution expired
     # ./lib/capybara/server.rb:77:in `block in boot'
     # ./lib/capybara/server.rb:77:in `boot'
     # ./spec/server_spec.rb:57:in `block (2 levels) in <top (required)>'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example.rb:114:in `instance_eval'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example.rb:114:in `block in run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example.rb:254:in `with_around_each_hooks'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example.rb:111:in `run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example_group.rb:388:in `block in run_examples'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example_group.rb:384:in `map'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example_group.rb:384:in `run_examples'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/example_group.rb:369:in `run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/command_line.rb:28:in `map'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/command_line.rb:28:in `block in run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/reporter.rb:34:in `report'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/command_line.rb:25:in `run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/runner.rb:80:in `run'
     # /Users/jon/.rvm/gems/ruby-1.9.3-p194@abweb/gems/rspec-core-2.12.2/lib/rspec/core/runner.rb:17:in `block in autorun'
@jdelStrother jdelStrother Fix random timeouts during Server#boot
Rack::Handler::Thin.run() (and, by extension, Capybara.server.call())
can return immediately if another Thin handler is already running.  This
results in a race condition where @server_thread can finish running
before we evaluate `responsive?`.   The easiest solution seems to be to
just remove the check that @server_thread is alive - it's quite possible
that EventMachine has merged the new server into a previous one, so we don't
particularly care about the state of our own thread.
aa9cf9b
@jdelStrother

Argh. Sorry - I was updating my 'timeouts' branch with a new commit, and managed to delete the branch due to a typo, which github has interpreted as me closing the request. Can I re-add my new 'timeouts' branch (jdelStrother@aa9cf9b) to this PR, or should I open a new one?

@jdelStrother jdelStrother reopened this
@joliss
Collaborator

I don't know a way to re-add branches, except for using the hub command line utility perhaps. Your best bet is to open a new PR and make a note here. (No biggie - GitHub gets wedged like this sometimes.)

@jdelStrother

... and github finally noticed the new branch.

The new version replaces the server_thread ivar with a local variable, which seems appropriate, assuming we really don't need the @server_thread check in responsive?.

@raggi

Thin is not thread safe, and basically incompatible with Capybara at it's core. It can work in many practical scenarios, but this is one that it's really not built for. EventMachine is specifically and by design not thread safe, and really will never directly support this execution model.

The fixable problem is, you aren't using a synchronous runner in Thin::Handler.run, and this is breaching the expected API contract, from an execution order standpoint.

It is arguably more correct to make Rack::Handler::Thin#run block until that thin server instance shuts down, i.e. Thread.current.join(0.1) while server.running? after https://github.com/rack/rack/blob/master/lib/rack/handler/thin.rb#L16

@raggi

@jnicklas that check prevents race conditions in responsive?. This PR does not describe a bug in that code.

@jnicklas jnicklas closed this in 5b164d4
@jnicklas
Owner

@raggi: sounds sensible. I removed the thin handler from the default server startup, so hopefully fewer people will run into this in the future. If they do, they will have handle this themselves. For those who don't explicitly need eventmachine, Puma is a better choice these days anyway.

@betelgeuse betelgeuse referenced this pull request in thoughtbot/capybara-webkit
Closed

Capybara-webkit stops / hangs unexpectedly and inconsistently #399

@gabebw gabebw referenced this pull request from a commit in thoughtbot/fake_braintree
@gabebw gabebw Always use WEBrick as the Capybara server
Thin has a number of problems as the Capybara server handler:

* #54
* jnicklas/capybara#920

Capybara >= 2.0.2 can directly use WEBrick as its server handler, and in fact is
preferred [1]. It works and shouldn't have the same threading issues.

[1]: https://github.com/jnicklas/capybara/blob/master/History.md#version-202
7e53111
@gabebw gabebw referenced this pull request in thoughtbot/fake_braintree
Merged

Always use WEBrick as the Capybara server #69

@gabebw gabebw referenced this pull request from a commit in thoughtbot/fake_braintree
@gabebw gabebw Always use WEBrick as the Capybara server
Thin has a number of problems as the Capybara server handler:

* #54
* jnicklas/capybara#920

Capybara >= 2.0.2 can directly use WEBrick as its server handler, and in fact is
preferred (see link below). It works and shouldn't have the same threading
issues.

Link: https://github.com/jnicklas/capybara/blob/master/History.md#version-202
aa95887
@gabebw gabebw referenced this pull request from a commit in thoughtbot/fake_braintree
@gabebw gabebw Always use WEBrick as the Capybara server
Thin has a number of problems as the Capybara server handler:

* #54
* jnicklas/capybara#920

Capybara >= 2.0.2 can directly use WEBrick as its server handler, and in fact is
preferred (see link below). It works and shouldn't have the same threading
issues. WEBrick is also more convenient, since it's included in Ruby.

Link: https://github.com/jnicklas/capybara/blob/master/History.md#version-202
ddb70f4
@gabebw gabebw referenced this pull request from a commit in thoughtbot/fake_braintree
@gabebw gabebw Always use WEBrick as the Capybara server
Thin has a number of problems as the Capybara server handler:

* #54
* jnicklas/capybara#920

Capybara >= 2.0.2 can directly use WEBrick as its server handler, and in fact is
preferred (see link below). It works and shouldn't have the same threading
issues. WEBrick is also more convenient, since it's included in Ruby.

Link: https://github.com/jnicklas/capybara/blob/master/History.md#version-202
4ac4145
@gabebw gabebw referenced this pull request in thoughtbot/fake_braintree
Closed

Using gem causes Thin server to boot on Rspec, and can't quit specs #54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 26, 2012
  1. @jdelStrother

    Fix random timeouts during Server#boot

    jdelStrother authored
    Rack::Handler::Thin.run() (and, by extension, Capybara.server.call())
    can return immediately if another Thin handler is already running.  This
    results in a race condition where @server_thread can finish running
    before we evaluate `responsive?`.   The easiest solution seems to be to
    just remove the check that @server_thread is alive - it's quite possible
    that EventMachine has merged the new server into a previous one, so we don't
    particularly care about the state of our own thread.
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 5 deletions.
  1. +2 −5 lib/capybara/server.rb
View
7 lib/capybara/server.rb
@@ -36,7 +36,6 @@ def ports
def initialize(app, port=Capybara.server_port)
@app = app
@middleware = Middleware.new(@app)
- @server_thread = nil # supress warnings
@port = port
@port ||= Capybara::Server.ports[@app.object_id]
@port ||= find_available_port
@@ -55,8 +54,6 @@ def host
end
def responsive?
- return false if @server_thread && @server_thread.join(0)
-
res = Net::HTTP.start(host, @port) { |http| http.get('/__identify__') }
if res.is_a?(Net::HTTPSuccess) or res.is_a?(Net::HTTPRedirection)
@@ -70,11 +67,11 @@ def boot
unless responsive?
Capybara::Server.ports[@app.object_id] = @port
- @server_thread = Thread.new do
+ server_thread = Thread.new do
Capybara.server.call(@middleware, @port)
end
- Timeout.timeout(60) { @server_thread.join(0.1) until responsive? }
+ Timeout.timeout(60) { server_thread.join(0.1) until responsive? }
end
rescue TimeoutError
raise "Rack application timed out during boot"
Something went wrong with that request. Please try again.