search

celluloid / reel

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

Refactor Reel::Server, refactor/retrofit/abandon SocketMixin #121

Closed digitalextremist closed 10 years ago

digitalextremist commented 10 years ago

Closes #114, Closes #119, Closes #120

stouset commented 10 years ago

Shouldn't the class names be updated to reflect their new location? e.g., Reel::Server::SSL, Reel::Server::UNIX, etc.?

digitalextremist commented 10 years ago

Didn't want to assume @tarcieri would want the Reel::Server* change, but I am open to that and it's not a huge change to add to the PR. I followed the queue of Reel::SSLServer

stouset commented 10 years ago

It looks like the SSLServer errors are due to the fact that the SSLServer no longer serves HTTP, but I'm not absolutely certain yet.

tarcieri commented 10 years ago

In general I think it'd be good if you left the SocketMixin refactoring out of this PR and focused just on changing the class names

digitalextremist commented 10 years ago

@tarcieri about to make all the requested changes in the next few minutes. Can we settle on keeping SocketMixin and just having that be the :TCP_NODELAY optimization for now, with no OS specific pieces?

My justification is that we have two TCPServer implementations now, with both deoptimization and optimization. It could just as easily be ServerMixins but since it handles a TCPSocket I kept SocketMixins

Starting on those changes.

digitalextremist commented 10 years ago

@tarcieri Ok, code convention changes made, and put up proposed compromise for SocketMixin without OS-specific code, and without corking.

digitalextremist commented 10 years ago

@tarcieri done and done

digitalextremist commented 10 years ago

@stouset not seeing the problem you mention theoretically, regarding SSLServer ... I referenced the @tarcieri code for how this began and right up to the major underlying changes which broke #supervise_as, host and port were the expected parameters, and SSLServer was instantiated when you wanted an SSL server. Beforehand, it did basically everything, but now it can super the remainder of its duplicated procedures.

digitalextremist commented 10 years ago

Made all requested changes per naming, spacing, and the rest. Testing changes. Oop, found an issue.

digitalextremist commented 10 years ago

As it stands, even if I change to < Reel::Server vs. < Server I am getting this in Reel::Server::SSL:

NameError: uninitialized constant Celluloid::IO::Server
    org/jruby/RubyModule.java:2684:in `const_missing'
    /mu/penultimatix/reel/lib/reel/server/ssl.rb:39:in `initialize'
    org/jruby/RubyKernel.java:1961:in `public_send'
    /mu/penultimatix/celluloid/lib/celluloid/calls.rb:26:in `dispatch'
    /mu/penultimatix/celluloid/lib/celluloid/calls.rb:63:in `dispatch'
    /mu/penultimatix/celluloid/lib/celluloid/cell.rb:60:in `invoke'
    /mu/penultimatix/celluloid/lib/celluloid/cell.rb:71:in `task'
    /mu/penultimatix/celluloid/lib/celluloid/actor.rb:388:in `task'
    /mu/penultimatix/celluloid/lib/celluloid/tasks.rb:55:in `initialize'
    /mu/penultimatix/celluloid/lib/celluloid/tasks.rb:47:in `initialize'
    /mu/penultimatix/celluloid/lib/celluloid/tasks/task_fiber.rb:15:in `create'
    (celluloid):0:in `remote procedure call'
    /mu/penultimatix/celluloid/lib/celluloid/calls.rb:92:in `value'
    /mu/penultimatix/celluloid/lib/celluloid/proxies/sync_proxy.rb:33:in `method_missing'
    /mu/penultimatix/celluloid/lib/celluloid/proxies/cell_proxy.rb:17:in `_send_'
    /mu/penultimatix/celluloid/lib/celluloid.rb:177:in `new_link'
    /mu/penultimatix/celluloid/lib/celluloid/supervision_group.rb:145:in `start'
    /mu/penultimatix/celluloid/lib/celluloid/supervision_group.rb:133:in `initialize'
    /mu/penultimatix/celluloid/lib/celluloid/supervision_group.rb:82:in `add'
    /mu/penultimatix/celluloid/lib/celluloid/supervision_group.rb:73:in `supervise_as'
    /mu/penultimatix/celluloid/lib/celluloid/calls.rb:26:in `public_send'
    /mu/penultimatix/celluloid/lib/celluloid/calls.rb:26:in `dispatch'
    /mu/penultimatix/celluloid/lib/celluloid/calls.rb:63:in `dispatch'
    /mu/penultimatix/celluloid/lib/celluloid/cell.rb:60:in `invoke'
    /mu/penultimatix/celluloid/lib/celluloid/cell.rb:71:in `task'
    /mu/penultimatix/celluloid/lib/celluloid/actor.rb:388:in `task'
    /mu/penultimatix/celluloid/lib/celluloid/tasks.rb:55:in `initialize'
    /mu/penultimatix/celluloid/lib/celluloid/tasks.rb:47:in `initialize'
    /mu/penultimatix/celluloid/lib/celluloid/tasks/task_fiber.rb:15:in `create'
Terminating task: type=:call, meta={:method_name=>:initialize}, status=:callwait
digitalextremist commented 10 years ago

Wait! the above issue was my stupid mistake in a search/replace.

digitalextremist commented 10 years ago

All clear on naming change, across the board.

digitalextremist commented 10 years ago

@stouset everything passes except the Reel::Server::UNIX piece. Not very familiar with test specs, but did the best I could. I broke apart server_spec.rb into unix_server_spec.rb and http_server_spec.rb and will need comments on how to test Reel::Server::UNIX /cc @tarcieri

stouset commented 10 years ago

On it.

digitalextremist commented 10 years ago

@stouset thanks - if you can quickly explain what I did that broke the testing mechanisms I'd appreciate that too.

digitalextremist commented 10 years ago

@stouset if you can pull my branch in to one of your forks and add/update the specs I will pull your branch into this one

stouset commented 10 years ago

PR submitted.

TL;DR, you were trying to connect a client to the socket before a server had created it to listen on (and then passing that client as a parameter to the server, rather than the path of the socket to the server).

stouset commented 10 years ago

Getting intermittent test failures.

Failures:

  1) Reel::Response::Writer streams static files
     Failure/Error: expect(buf).to eq expected_response

       expected: "HTTP/1.1 200 OK\r\nContent-Length: 56\r\n\r\nThis is an example of a static file being served by Reel"
            got: "HTTP/1.1 200 OK\r\nContent-Length: 56\r\n\r\n"

       (compared using ==)

       Diff:
       @@ -1,5 +1,4 @@
        HTTP/1.1 200 OK
        Content-Length: 56

       -This is an example of a static file being served by Reel

     # ./vendor/bundler/gems/rspec-expectations-2.14.4/lib/rspec/expectations/fail_with.rb:32:in `fail_with'
     # ./vendor/bundler/gems/rspec-expectations-2.14.4/lib/rspec/expectations/handler.rb:34:in `handle_matcher'
     # ./vendor/bundler/gems/rspec-expectations-2.14.4/lib/rspec/expectations/expectation_target.rb:34:in `to'
     # ./spec/reel/response/writer_spec.rb:23:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:36:in `with_socket_pair'
     # ./spec/reel/response/writer_spec.rb:8:in `block (2 levels) in <top (required)>'
digitalextremist commented 10 years ago

@stouset thanks, brought it in, and I appreciate the explanation. Does http_server_spec.rb look OK?

digitalextremist commented 10 years ago

Ultra-stupid question, but what's the command to run the tests locally versus waiting for travis?

tarcieri commented 10 years ago

@digitalextremist rake or rspec

digitalextremist commented 10 years ago

@tarcieri thanks!

digitalextremist commented 10 years ago

Awesome! Tests looking good. Not sure if adding a #supervise test is worth it, but it doesn't seem like it would be a waste. A test for every form of instantiation if not now seems valid down the line.

digitalextremist commented 10 years ago

Rather -- all tests looking good except exotic failure under jruby-head at Reel::Server::UNIX test.

digitalextremist commented 10 years ago

@stouset does this address the Reel::Response::Writer intermittent failure, or?

stouset commented 10 years ago

Yep. All tests green now. 

Stephen Touset stephen@touset.org

On Wed, Nov 13, 2013 at 3:13 PM, digitalextremist notifications@github.com wrote:

@stouset does this address the Reel::Response::Writer intermittent failure, or?

Reply to this email directly or view it on GitHub: https://github.com/celluloid/reel/pull/121#issuecomment-28443889

digitalextremist commented 10 years ago

@stouset I'm seeing this on both jRuby test sets:

Failures:
  1) Reel::Server::UNIX allows connections over UNIX sockets
     Failure/Error: expect(response.body).to eq(self.response_body)
     NoMethodError:
       undefined method `body' for nil:NilClass
     # ./spec/reel/unix_server_spec.rb:33:in `(root)'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/shared/tmpdir.rb:0:in `create'
     # ./spec/reel/unix_server_spec.rb:20:in `(root)'
     # org/jruby/RubyBasicObject.java:1735:in `instance_eval'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example.rb:114:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example.rb:254:in `with_around_each_hooks'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example.rb:111:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example_group.rb:390:in `run_examples'
     # org/jruby/RubyArray.java:2417:in `map'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example_group.rb:386:in `run_examples'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example_group.rb:371:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:28:in `run'
     # org/jruby/RubyArray.java:2417:in `map'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:28:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/reporter.rb:58:in `report'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:25:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/runner.rb:80:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/runner.rb:17:in `autorun'
digitalextremist commented 10 years ago

@stouset seems like it's the placement of the test within ensure

digitalextremist commented 10 years ago

@stouset, the ensure placement seems to have fixed that piece, now it's this:

Failures:
  1) Reel::Server::UNIX allows connections over UNIX sockets
     Failure/Error: response = Net::HTTPResponse.read_new(sock)
     Errno::ECONNRESET:
       Connection reset by peer - Connection reset by peer
     # org/jruby/RubyIO.java:2671:in `read_nonblock'
     # org/jruby/RubyBasicObject.java:1709:in `__send__'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/1.9/net/protocol.rb:141:in `rbuf_fill'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/1.9/net/protocol.rb:122:in `readuntil'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/1.9/net/protocol.rb:132:in `readline'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/1.9/net/http.rb:2570:in `read_status_line'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/1.9/net/http.rb:2559:in `read_new'
     # ./spec/reel/unix_server_spec.rb:28:in `(root)'
     # /home/travis/.rvm/rubies/jruby-1.7.4-d19/lib/ruby/shared/tmpdir.rb:0:in `create'
     # ./spec/reel/unix_server_spec.rb:20:in `(root)'
     # org/jruby/RubyBasicObject.java:1735:in `instance_eval'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example.rb:114:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example.rb:254:in `with_around_each_hooks'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example.rb:111:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example_group.rb:390:in `run_examples'
     # org/jruby/RubyArray.java:2417:in `map'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example_group.rb:386:in `run_examples'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/example_group.rb:371:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:28:in `run'
     # org/jruby/RubyArray.java:2417:in `map'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:28:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/reporter.rb:58:in `report'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:25:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/runner.rb:80:in `run'
     # /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/rspec-core-2.14.7/lib/rspec/core/runner.rb:17:in `autorun'

Still under jRuby only.

stouset commented 10 years ago

Found the root issue. When UNIXServer#accept is called, it ends up going down to NIO::Selector#register(io, :r), and JRuby doesn't support the r mode on that object.

stouset commented 10 years ago

@tarcieri You may have a better idea of how to proceed at this point. This is way deeper in the guts of Celluloid than I'm comfortable with.

digitalextremist commented 10 years ago

@stouset nice digging! @tarcieri shall I take all Server::UNIX code out into another branch, or can this be worked around or solved simply?

I also notice a strange error in jruby-head so I wonder if UNIX sockets are generally touchy under jRuby?

tarcieri commented 10 years ago

Let me talk to Charlie about this. nio4r may need some special case handling for UNIXSockets on Java.

Guess nobody has ever actually hit the selector when using a UNIXSocket before.

@halorgium we should probably force selection for reads, writes, and the already broken r/w state in the Celluloid::IO tests

On Wednesday, November 13, 2013, Stephen Touset wrote:

@tarcieri https://github.com/tarcieri You may have a better idea of how to proceed at this point. This is way deeper in the guts of Celluloid than I'm comfortable with.

— Reply to this email directly or view it on GitHubhttps://github.com/celluloid/reel/pull/121#issuecomment-28458050 .

Tony Arcieri

digitalextremist commented 10 years ago

@tarcieri since it has been a while on this, ought I move Server::UNIX out into another branch and pull the rest of the code without it? Seems like this core theory change for how Server becomes Server::* ought to get into the bloodstream for Reel so adjustments can be made for old code out there.

stouset commented 10 years ago

@digitalextremist If you mark the failing test as skipped on JRuby, I believe @tarcieri will merge the PR.

stouset commented 10 years ago

You're the owner of the branch, so I'm not able to push to it. :(

tarcieri commented 10 years ago

@digitalextremist feel free to create these kinds of branches under celluloid/reel now directly.

@stouset can you send a PR to this branch?

digitalextremist commented 10 years ago

Cool, right on. Will do @tarcieri.

Yeah, please do send a PR to this branch @stouset. I'll definitely merge that in.

digitalextremist commented 10 years ago

I moved the Server::UNIX code out into its own branch and removed it from this one.

digitalextremist commented 10 years ago

Travis seems to be having trouble, it doesn't seem like it's running the tests and failing. It's just failing. Will try again later.

digitalextremist commented 10 years ago

@stouset you can post test changes to the unix_server branch: https://github.com/celluloid/reel/tree/unix_server

digitalextremist commented 10 years ago

Travis' failure was actually my mistake for not pulling out require 'reel/server/unix' so it ought to completely pass now... except for the rbx binary failing. Yeah -- just verified all good.

tarcieri commented 10 years ago

In addition to the nits, we should probably try fixing the rbx build failure so the build is green when it passes.

The simplest way to do this for now is probably adding rbx to allowed failures. If you can figure out how to get it to build successfully on rbx, all the better.

digitalextremist commented 10 years ago

Roger that, on all nits. Those ought to be gone in 90 minutes or so when I get back to my desk.

Will also look into rbx binary failures or mark it allowed to fail, but yes I'd rather actually fix it.

tarcieri commented 10 years ago

Awesome, thank you! :smile_cat:

digitalextremist commented 10 years ago

And I will commit without ( spaces ) in the future.

digitalextremist commented 10 years ago

@tarcieri thanks for the great notes above. Now looking at the rbx issue. All nits picked off.

Looking forward to pulling the trigger on this one, hopefully today.

digitalextremist commented 10 years ago

The rbx issue is discussed here: https://github.com/travis-ci/travis-ci/issues/1641

Working through the implications to Reel

tarcieri commented 10 years ago

Try changing it from rbx-19mode to rbx-2

digitalextremist commented 10 years ago

Yeah, I did just plain rbx but I will do rbx-2 also.