-
Notifications
You must be signed in to change notification settings - Fork 87
Refactor Reel::Server, refactor/retrofit/abandon SocketMixin #121
Changes from 15 commits
169a7fd
f19c2d1
6fa45f3
d1fab02
9f68968
c5c34cb
41f0289
f5ed50a
0546ecc
38dfccf
c1f0a7f
fe9f290
6975e29
516bf12
5337bdf
3d1fdd2
5eb4ebf
c7a05d8
39fce89
cc5368c
38a7a4a
aabd214
1f83b88
ce2d90d
f1a8ecf
fab05f8
0d47f52
a64e4ba
081d989
75bc958
9dc37aa
8bb67b9
e985d41
fd1f40c
c4b0f6d
d6891a6
4d3d033
9501d5b
eb461c8
f426928
511ca32
fd8c8dc
4659b4e
37dd5fc
99c2f7a
e977bcd
18f32f3
feae2f7
a91f046
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,8 @@ traditional multithreaded blocking I/O support too. | |
[nio4r]: https://github.com/celluloid/nio4r | ||
|
||
Connections to Reel can be either non-blocking and handled entirely within | ||
the Reel::Server thread, or the same connections can be dispatched to worker | ||
the Reel::Server thread ( handling HTTP, SSL, or UNIX sockets ), | ||
or the same connections can be dispatched to worker | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: word wrapping (please wrap to ~80 columns) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's within 80, yes, but it's underindented relative to the rest of the text. |
||
threads where they will perform ordinary blocking IO. Reel provides no | ||
built-in thread pool, however you can build one yourself using Celluloid.pool, | ||
or because Celluloid already pools threads to begin with, you can simply use | ||
|
@@ -132,7 +133,7 @@ Reel lets you pass a block to initialize which receives connections: | |
```ruby | ||
require 'reel' | ||
|
||
Reel::Server.supervise("0.0.0.0", 3000) do |connection| | ||
Reel::HTTPServer.supervise("0.0.0.0", 3000) do |connection| | ||
# Support multiple keep-alive requests per connection | ||
connection.each_request do |request| | ||
# WebSocket support | ||
|
@@ -163,7 +164,7 @@ You can also subclass Reel, which allows additional customizations: | |
```ruby | ||
require 'reel' | ||
|
||
class MyServer < Reel::Server | ||
class MyServer < Reel::HTTPServer | ||
def initialize(host = "127.0.0.1", port = 3000) | ||
super(host, port, &method(:on_connection)) | ||
end | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ def remote_host | |
# NOTE: Celluloid::IO does not yet support non-blocking reverse DNS | ||
socket.peeraddr(true)[2] | ||
end | ||
|
||
end | ||
|
||
module RequestMixin | ||
|
@@ -60,4 +61,51 @@ def fragment | |
end | ||
|
||
end | ||
|
||
module SocketMixin | ||
|
||
# Optimizations possible, depending on OS: | ||
|
||
# TCP_NODELAY: prevent TCP packets from being buffered | ||
# TCP_CORK: TODO: tersely describe | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cork is not being used correctly. Can you please remove it? |
||
# SO_REUSEADDR: TODO: tersely describe | ||
|
||
if RUBY_PLATFORM =~ /linux/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you leave this out for the time being? |
||
|
||
# Only Linux supports the mix of socket behaviors given in these optimizations. | ||
# Beaware, certain optimizations may work individually off Linux; not together. | ||
def optimize_socket socket | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No "Seattle style" parameters please. Please add parens around |
||
if socket.is_a? TCPSocket | ||
socket.setsockopt( Socket::IPPROTO_TCP, :TCP_NODELAY, 1 ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remove the spaces around the parameters? |
||
socket.setsockopt( Socket::IPPROTO_TCP, 3, 1 ) # TCP_CORK | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove CORK |
||
socket.setsockopt( Socket::SOL_SOCKET, Socket::SO_REUSEADDR, 1 ) | ||
end | ||
end | ||
|
||
def deoptimize_socket socket | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
if socket.is_a? TCPSocket | ||
socket.setsockopt( Socket::IPPROTO_TCP, :TCP_NODELAY, 1 ) | ||
socket.setsockopt( Socket::IPPROTO_TCP, 3, 1 ) # TCP_CORK | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
socket.setsockopt( Socket::SOL_SOCKET, Socket::SO_REUSEADDR, 1 ) | ||
end | ||
end | ||
|
||
else | ||
|
||
# If the underying OS is not Linux, apply the remaining available optimizations. | ||
def optimize_socket socket | ||
if socket.is_a? TCPSocket | ||
socket.setsockopt( Socket::IPPROTO_TCP, :TCP_NODELAY, 1 ) | ||
end | ||
end | ||
|
||
def deoptimize_socket socket | ||
if socket.is_a? TCPSocket | ||
socket.setsockopt( Socket::IPPROTO_TCP, :TCP_NODELAY, 0 ) | ||
end | ||
end | ||
end | ||
|
||
end | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,71 +1,36 @@ | ||
module Reel | ||
# The Reel HTTP server class | ||
# The Reel server ( foundation ) class | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You sure like those I'd probably say this here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger, and yes, but I can keep in mind the convention in use and be less spaced out. |
||
# | ||
# This class is a Celluloid::IO actor which provides a bareboens HTTP server | ||
# For HTTPS support, use Reel::SSLServer | ||
# This class is a Celluloid::IO actor which provides a barebones server | ||
# which does not open a socket itself, it just begin handling connections once | ||
# initialized with a specific kind of protocol-based server. | ||
|
||
# For specific protocol support, use: | ||
|
||
# Reel::HTTPServer | ||
# Reel::SSLServer | ||
# Reel::UNIXServer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be Reel::Server::UNIX |
||
|
||
class Server | ||
include Celluloid::IO | ||
include SocketMixin | ||
|
||
# How many connections to backlog in the TCP accept queue | ||
DEFAULT_BACKLOG = 100 | ||
|
||
execute_block_on_receiver :initialize | ||
finalizer :shutdown | ||
|
||
# Allow the existing `new` to be called, even though we will | ||
# replace it with a default version that creates HTTP servers over | ||
# TCP sockets. | ||
# | ||
class << self | ||
alias_method :_new, :new | ||
protected :_new | ||
end | ||
|
||
# Create a new Reel HTTP server | ||
# | ||
# @param [String] host address to bind to | ||
# @param [Fixnum] port to bind to | ||
# @option options [Fixnum] backlog of requests to accept | ||
# @option options [true] spy on the request | ||
# | ||
# @return [Reel::SSLServer] Reel HTTPS server actor | ||
# | ||
# ::new was overridden for backwards compatibility. The underlying | ||
# #initialize method now accepts a `server` param that is | ||
# responsible for having established the bi-directional | ||
# communication channel. ::new uses the existing (sane) default of | ||
# setting up the TCP channel for the user. | ||
# | ||
def self.new(host, port, options = {} , &callback) | ||
server = Celluloid::IO::TCPServer.new(host, port) | ||
backlog = options.fetch(:backlog, DEFAULT_BACKLOG) | ||
|
||
# prevent TCP packets from being buffered | ||
server.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) | ||
server.listen(backlog) | ||
|
||
self._new(server, options, &callback) | ||
end | ||
|
||
# Create a Reel HTTP server over a UNIX socket. | ||
# | ||
# @param [String] socket_path path to the UNIX socket | ||
# @option options [true] spy on the request | ||
# | ||
def self.unix(socket_path, options = {}, &callback) | ||
server = Celluloid::IO::UNIXServer.new(socket_path) | ||
|
||
self._new(server, options, &callback) | ||
end | ||
|
||
def initialize(server, options = {}, &callback) | ||
def initialize(server, options={}, &callback) | ||
@spy = STDOUT if options[:spy] | ||
@server = server | ||
@options = options | ||
@callback = callback | ||
@server = server | ||
|
||
@server.listen(options.fetch(:backlog, DEFAULT_BACKLOG)) | ||
|
||
async.run | ||
end | ||
|
||
end | ||
|
||
def shutdown | ||
@server.close if @server | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
module Reel | ||
class HTTPServer < Server | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This breaks the mapping between class names and file names. I'd suggest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok we were waiting for this confirmation, I will refactor based on that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed naming and will change all other references to HTTPServer and SSLServer after testing. |
||
|
||
execute_block_on_receiver :initialize | ||
|
||
# Create a new Reel HTTPS server | ||
# | ||
# @param [String] host address to bind to | ||
# @param [Fixnum] port to bind to | ||
# @option options [Fixnum] backlog of requests to accept | ||
# | ||
# @return [Reel::HTTPServer] Reel HTTP server actor | ||
def initialize(host, port, options={}, &callback) | ||
optimize_socket server = Celluloid::IO::TCPServer.new(host, port) | ||
options.merge!({ :host => host, :port => port }) | ||
super(server, options, &callback) | ||
end | ||
|
||
def shutdown | ||
deoptimize_socket @server | ||
super | ||
end | ||
|
||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HTTPServer looks like it can no longer be composed with SSLServer, which was the whole point of the original approach. :( |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,8 @@ class SSLServer < Server | |
# @option options [String] :key the server's SSL key | ||
# | ||
# @return [Reel::SSLServer] Reel HTTPS server actor | ||
def initialize(server, options = {}, &callback) | ||
def initialize(host, port, options={}, &callback) | ||
|
||
# Ideally we can encapsulate this rather than making Ruby OpenSSL a | ||
# mandatory part of the Reel API. It would be nice to support | ||
# alternatives (e.g. Puma's MiniSSL) | ||
|
@@ -31,16 +32,23 @@ def initialize(server, options = {}, &callback) | |
else OpenSSL::SSL::VERIFY_NONE | ||
end | ||
|
||
# wrap an SSLServer around the Reel::Server we've been given | ||
ssl_server = Celluloid::IO::SSLServer.new(server, ssl_context) | ||
optimize_socket @tcpserver = Celluloid::IO::TCPServer.new(host, port) | ||
|
||
server = Celluloid::IO::SSLServer.new(@tcpserver, ssl_context) | ||
options.merge!({ :host => host, :port => port }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary if the TCPServer has already been established on the host/port? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is where the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I mean the merging of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stouset I do that so the In what I've seen, implementations sometimes need to know the underlying options the server is configured for to know how to setup environments properly request by request, or connection by connection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style nit: this (and the others) should be: options.merge!(host: host, port: port) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it // |
||
|
||
super(server, options, &callback) | ||
end | ||
|
||
super(ssl_server, options, &callback) | ||
def shutdown | ||
deoptimize_socket @tcpserver | ||
super | ||
end | ||
|
||
def run | ||
loop do | ||
begin | ||
socket = @server.accept | ||
socket = @handler.accept | ||
rescue OpenSSL::SSL::SSLError => ex | ||
Logger.warn "Error accepting SSLSocket: #{ex.class}: #{ex.to_s}" | ||
retry | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
module Reel | ||
class UNIXServer < Server | ||
|
||
execute_block_on_receiver :initialize | ||
|
||
# Create a new Reel HTTPS server | ||
# | ||
# @option options [String] socket path to bind to | ||
# @option options [Fixnum] backlog of requests to accept | ||
# | ||
# @return [Reel::UNIXServer] Reel UNIX server actor | ||
def initialize(socket_path, options={}, &callback) | ||
server = Celluloid::IO::UNIXServer.new(socket_path) | ||
options[:socket_path] = socket_path | ||
super(server, options, &callback) | ||
end | ||
|
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no spaces inside parens please