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

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

Open
damned opened this issue Jul 22, 2016 · 7 comments
Open

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

damned opened this issue Jul 22, 2016 · 7 comments
Assignees

Comments

@damned
Copy link

damned commented Jul 22, 2016

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 celluloid/reel#229

@damned
Copy link
Author

damned commented Jul 26, 2016

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
Copy link
Author

damned commented Jul 30, 2016

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
Copy link
Owner

kenichi commented Aug 8, 2016

@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 kenichi self-assigned this Aug 8, 2016
@kenichi
Copy link
Owner

kenichi commented Aug 24, 2016

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
Copy link

respire commented Jan 25, 2017

@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
Copy link
Owner

kenichi commented Jan 29, 2017

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
Copy link

respire commented Jan 30, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants