celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

0.6.1: Actor crashed! can't change state from 'closed' to 'headers' #229

Open damned opened 8 years ago

damned commented 8 years ago

We're using angelo intermittently (but usually) getting:

Actor crashed! ArgumentError: Reel::Request::StateMachine can't change state from 'closed' to 'headers'

when reel is handling a POST request generated from Ruby Net::HTTP.

The problem seems to originate from the Reel code, that's why i'm raising issue here - i'll raise a referencing issue in Angelo to track it...

What we found so far:

There seems to be a logic hole between the Connection and the Parser which is exposed when the headers are received for a non-chunked request, but the body has not yet arrived. This happens often because Net::HTTP sends the headers and body in two separate packets.

When the headers are first received, Parser#current_request parses the header and sets the request to @currently_reading (which exists to support "streaming"). Although the full message has not yet been received the connection will try to #respond to the request - #respond in turn does not recognise that we are "streaming" and transitions the request to closed.

A simple test verifies that this problem is fixed by removing @currently_reading from Parser#current_request so that it ensures the request is fully complete before continuing - presumably this simplistic change would break "streaming" in reel.

Full stack:

E, [2016-07-22T12:20:03.680392 #12294] ERROR -- : Actor crashed! ArgumentError: Reel::Request::StateMachine can't change state from 'closed' to 'headers', only to: /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-fsm-0.20.5/lib/celluloid/fsm.rb:114:in validate_and_sanitize_new_state' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-fsm-0.20.5/lib/celluloid/fsm.rb:90:intransition' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/reel-0.6.1/lib/reel/connection.rb:58:in request' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/reel-0.6.1/lib/reel/connection.rb:73:ineach_request' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/angelo-0.5.0/lib/angelo/server.rb:27:in on_connection' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/reel-0.6.1/lib/reel/server.rb:50:inhandle_connection' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/calls.rb:28:in public_send' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/calls.rb:28:indispatch' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/call/async.rb:7:in dispatch' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/cell.rb:50:inblock in dispatch' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/cell.rb:76:in block in task' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/actor.rb:339:inblock in task' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/task.rb:44:in block in initialize' /home/dmoore/.rvm/gems/ruby-2.3.1@tw.website/gems/celluloid-0.17.3/lib/celluloid/task/fibered.rb:14:inblock in create'

damned commented 8 years ago

Added code to reproduce via angelo at https://github.com/kenichi/angelo/issues/70

damned commented 8 years ago

simplest code i can make to reproduce (is intermittent, loop should mean it usually occurs) reel 0.6.2, ruby 2.3.1, ubuntu 14.04

require 'celluloid/autostart'
require_relative 'lib/reel'

Reel::Server::HTTP.supervise('127.0.0.1', 7077) do |connection|
  connection.each_request do |request|
    request.respond :ok, 'hello'
  end
end

require 'net/http'

(1..10).each do
  http = Net::HTTP.new '127.0.0.1', 7077
  http.post '/somepath', 'somedata'
end
kenichi commented 8 years ago

@damned i have a feeling this is angelo's fault, as the repro case above can be solved with one line addition:

Reel::Server::HTTP.supervise('127.0.0.1', 7077) do |connection|
  connection.each_request do |request|

    # add this
    request.body.to_s

    request.respond :ok, 'hello'
  end
end

see https://github.com/celluloid/reel/issues/150#issuecomment-45136694.

damned commented 8 years ago

hiya, i understand your point.

i feel angelo would be much more intuitive if did not allow this to hit the end user :)

whether reel itself should intermittently fail with such message for such a (not very obvious) usage error i guess is a subjective matter - my feeling was that reel is not well-behaved in this case.