kenichi / angelo

Sinatra-like DSL for Reel that supports WebSockets and SSE
Other
303 stars 23 forks source link

0.5.0 Actor crashed! handling POST request from Net::HTTP #70

Open damned opened 8 years ago

damned commented 8 years ago

Using reel 0.6.1 got

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

when handling Net::HTTP POST

Raised issue in reel https://github.com/celluloid/reel/issues/229

damned commented 8 years ago

Just found that the work around to always ensure the request is fully read by starting the breaking responder block with:

params.dup

as suggested in #21 works around this bug too.

damned commented 8 years ago

here is the simplest reproduction i can find for problem (angelo 0.5.0, reel 0.6.2, ruby 2.3.1, ubuntu 14.04):

require 'angelo'

class PoorlyServer < Angelo::Base
  post '/somepath' do
    'some body'
  end
end
PoorlyServer.run '127.0.0.1', 7077, {}, false

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 thanks for your work on this, sorry I haven't responded until now. I'm working with the reel community to try to address this issue.

kenichi commented 8 years ago

see my comment over on reel. interesting about params.dup... let me investigate that a bit more.

so, this should work even without the .dup, just by calling params, the request body is fully read.

all of the complexity behind Connection#request is apparently in support of HTTP Pipelining, which according to wikipedia, isn't supported by current browsers.

respire commented 7 years ago

@damned @kenichi IMO, the problem is connection#each_request will yield request once headers are parsed.

if you patch Reel::Request::Parser#current_request in you local reel like this reel/request/parser.rb


      def current_request
        until @currently_responding
          readpartial
        end
        @currently_responding
      end

your test will pass.

➜  async_your_app be ruby only_header.rb
I, [2017-01-25T17:29:03.906500 #497]  INFO -- : Celluloid 0.17.3 is running in BACKPORTED mode. [ http://git.io/vJf3J ]
I, [2017-01-25T17:29:04.139348 #497]  INFO -- : Angelo 0.5.0
I, [2017-01-25T17:29:04.139405 #497]  INFO -- : listening on 127.0.0.1:7077
I, [2017-01-25T17:29:04.141986 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.143539 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.144736 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.145882 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.147115 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.148245 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.149461 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.150600 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.151981 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9
I, [2017-01-25T17:29:04.153145 #497]  INFO -- : 127.0.0.1 - - "POST /somepath HTTP/1.1" 200 9

maybe i can send a fix from angelo side or reel side.

kenichi commented 7 years ago

hi @respire thanks - i agree that is the problem, and i like the simplicity of the fix, but it causes reel specs to fail/hang on the pipelining stuff. i'm not opposed to taking that stuff out, but the larger reel org would need to consent. again, i don't think it's supported by modern browsers, and h2 makes h1.1 pipelining moot.

respire commented 7 years ago

hi @kenichi - thanks for your reply! actually it's just an example patch to indicate the problem. it's not the final fix, and I agree that it's better to fix from angelo side. i'll send another PR from angelo side when I have time.