Rev talk - Aug 2009 - making listeners from existing objects (w/preliminary patch)

Hi,

I''m toying around with projects that receive/inherit file descriptors
from other processes.  So in those cases I''ll end up with a ::TCPServer
object already in my hands (usually from ::TCPServer.for_fd(fileno))
that I''d like to use with Revactor.

The following is a first step/patch into making rev/revactor able to
accept existing server objects.  See examples t0.rb + t1.rb below on how
they''d be used).

diff --git a/lib/rev/listener.rb b/lib/rev/listener.rb
index 9b17ce7..aedc11a 100644
--- a/lib/rev/listener.rb
+++ b/lib/rev/listener.rb
@@ -49,11 +49,11 @@ module Rev
     #  :backlog - Max size of the pending connection queue (default 1024)
     #  :reverse_lookup - Retain BasicSocket''s reverse DNS
functionality (default false)
     #
-    def initialize(addr, port, options = {})
+    def initialize(addr, port = nil, options = {})
       BasicSocket.do_not_reverse_lookup = true unless options[:reverse_lookup]
       options[:backlog] ||= DEFAULT_BACKLOG
       
-      listen_socket = ::TCPServer.new(addr, port)
+      listen_socket = ::TCPServer === addr ? addr : ::TCPServer.new(addr, port)
       listen_socket.instance_eval { listen(options[:backlog]) }
       super(listen_socket)
     end
---

In this example, t0.rb binds the socket (127.0.0.1:8888), and then
executes t1.rb immediately.  t1.rb does the actual work with the
socket that t0.rb bound.

------------------ t0.rb -------------------
#!/home/ew/ruby-1.9/bin/ruby
require ''socket''
s = TCPServer.new(''127.0.0.1'', 8888)

# give the child process a file descriptor to inherit:
exec ''./t1.rb'', s.fileno.to_s
------------------ t1.rb -------------------
#!/home/ew/ruby-1.9/bin/ruby
require ''revactor''
fileno = ARGV.first.to_i
s = TCPServer.for_fd(fileno)
listener = Revactor::TCP.listen(s, nil)
p [ :inherited, listener ]

# the echo server example shipped with revactor:
loop do
  # Accept an incoming connection and start a new Actor
  # to handle it
  Actor.spawn(listener.accept) do |sock|
    puts "#{sock.remote_addr}:#{sock.remote_port} connected"

    # Begin echoing received data
    loop do
      begin
        # Write everything we read
        sock.write sock.read
      rescue EOFError
        puts "#{sock.remote_addr}:#{sock.remote_port} disconnected"

        # Break (and exit the current actor) if the connection
        # is closed, just like with a normal Ruby socket
        break
      end
    end
  end
end
---

Please let me know what you think, thanks!

I''ll make a proper patch+commit message that also works for UNIXServer
if you all like it.

-- 
Eric Wong

Tony Arcieri <tony at medioh.com> wrote:
> That looks mostly good, aside from the fact that there''s no check
for if a
> user passes a bind address but no port.  If you''d like to put
together a
> patch or send me a pull request on Github I can incorporate this.
I''ve added the check and also made UNIXListener behave the same way
I''ve also added a spec for UNIXListener since it''s easier to
test
UNIXListener than TCPListener.

I''ve pushed out to git://yhbt.net/rev.git.  I''m not
comfortable using
Github for multiple reasons.  I''ll also reply to this message with the
patches so you can comment on them if needed.  There''s also a second
patch in the series that I think will be useful:

Eric Wong (2):
      Allow listeners to build off existing {UNIX,TCP}Server objects
      Add Rev::Listener#fileno to access the numeric descriptor
---
 lib/rev/listener.rb        |   23 +++++++++++++++++++----
 spec/unix_listener_spec.rb |   25 +++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 4 deletions(-)

-- 
Eric Wong

Instead of having these objects to bind new descriptors at
initialization, allow them to be initialized off existing
TCPServer or UNIXServer objects.  This is useful for servers
that are handed numeric file descriptors and create *Server
objects out of them via {TCP,UNIX}Server.for_fd

Signed-off-by: Eric Wong <normalperson at yhbt.net>
---
 lib/rev/listener.rb        |   16 +++++++++++++---
 spec/unix_listener_spec.rb |   27 +++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 spec/unix_listener_spec.rb

diff --git a/lib/rev/listener.rb b/lib/rev/listener.rb
index 9b17ce7..3634d95 100644
--- a/lib/rev/listener.rb
+++ b/lib/rev/listener.rb
@@ -49,11 +49,19 @@ module Rev
     #  :backlog - Max size of the pending connection queue (default 1024)
     #  :reverse_lookup - Retain BasicSocket''s reverse DNS
functionality (default false)
     #
-    def initialize(addr, port, options = {})
+    # If the specified address is an TCPServer object, it will ignore
+    # the port and :backlog option and create a new Rev::TCPListener out
+    # of the existing TCPServer object.
+    def initialize(addr, port = nil, options = {})
       BasicSocket.do_not_reverse_lookup = true unless options[:reverse_lookup]
       options[:backlog] ||= DEFAULT_BACKLOG
       
-      listen_socket = ::TCPServer.new(addr, port)
+      listen_socket = if ::TCPServer === addr
+        addr
+      else
+        raise ArgumentError, "port must be an integer" if nil == port
+        ::TCPServer.new(addr, port)
+      end
       listen_socket.instance_eval { listen(options[:backlog]) }
       super(listen_socket)
     end
@@ -63,8 +71,10 @@ module Rev
     # Create a new Rev::UNIXListener
     #
     # Accepts the same arguments as UNIXServer.new
+    # Optionally, it can also take anyn existing UNIXServer object
+    # and create a Rev::UNIXListener out of it.
     def initialize(*args)
-      super(::UNIXServer.new(*args))
+      super(::UNIXServer === args.first ? args.first : ::UNIXServer.new(*args))
     end
   end
 end
diff --git a/spec/unix_listener_spec.rb b/spec/unix_listener_spec.rb
new file mode 100644
index 0000000..281d149
--- /dev/null
+++ b/spec/unix_listener_spec.rb
@@ -0,0 +1,27 @@
+require File.dirname(__FILE__) + ''/../lib/rev''
+require ''tempfile''
+
+describe Rev::UNIXListener do
+
+  before :each do
+    @tmp = Tempfile.new(''rev_unix_listener_spec'')
+    File.unlink(@tmp.path).should == 1
+    File.exist?(@tmp.path).should == false
+  end
+
+  it "creates a new UNIXListener" do
+    listener = Rev::UNIXListener.new(@tmp.path)
+    File.socket?(@tmp.path).should == true
+  end
+
+  it "builds off an existing UNIXServer" do
+    unix_server = UNIXServer.new(@tmp.path)
+    File.socket?(@tmp.path).should == true
+    listener = Rev::UNIXListener.new(unix_server)
+    File.socket?(@tmp.path).should == true
+    listener.instance_eval {
+      @listen_socket.fileno.should == unix_server.fileno
+    }
+  end
+
+end
-- 
Eric Wong

It is useful to access the underlying file descriptor in case a
server needs to exec() a new process and share the file
descriptor information with its children (via the command-line
or environment variable).   The child could then call
*Server.for_fd on the file descriptor.

This also makes the existing unix_listener_spec easier
to implement.

Signed-off-by: Eric Wong <normalperson at yhbt.net>
---
 lib/rev/listener.rb        |    7 ++++++-
 spec/unix_listener_spec.rb |    4 +---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/rev/listener.rb b/lib/rev/listener.rb
index 3634d95..7069737 100644
--- a/lib/rev/listener.rb
+++ b/lib/rev/listener.rb
@@ -15,7 +15,12 @@ module Rev
       @listen_socket = listen_socket
       super(@listen_socket)
     end
-    
+
+    # Returns an integer representing the underlying numeric file descriptor
+    def fileno
+      @listen_socket.fileno
+    end
+
     # Close the listener
     def close
       detach if attached?
diff --git a/spec/unix_listener_spec.rb b/spec/unix_listener_spec.rb
index 281d149..a264cf7 100644
--- a/spec/unix_listener_spec.rb
+++ b/spec/unix_listener_spec.rb
@@ -19,9 +19,7 @@ describe Rev::UNIXListener do
     File.socket?(@tmp.path).should == true
     listener = Rev::UNIXListener.new(unix_server)
     File.socket?(@tmp.path).should == true
-    listener.instance_eval {
-      @listen_socket.fileno.should == unix_server.fileno
-    }
+    listener.fileno.should == unix_server.fileno
   end
 
 end
-- 
Eric Wong

Tony Arcieri <tony at medioh.com> wrote:
> Hi, sorry about the belated reply.
> 
> I can''t get this patch to reply.  Can you fork my repository on
github and
> send me a pull request, or attach the patchfile as an attachment rather
than
> pasting it into the body of the email?
Yeah, in the other email I noted I also cloned and pushed to
git://yhbt.net/rev.git  I''ll continue pushing to that if I come up with
other patches.
> patching file lib/rev/listener.rb
> Hunk #1 FAILED at 15.
> 1 out of 1 hunk FAILED -- saving rejects to file lib/rev/listener.rb.rej
> patching file spec/unix_listener_spec.rb
> Hunk #1 FAILED at 19.
> 1 out of 1 hunk FAILED -- saving rejects to file
> spec/unix_listener_spec.rb.rej
Odd, which email client do you use?

Some folks I know save the emails to a temporary mbox and run "git am"
against it.  I''ve also been using mutt with the following macros
defined
in my muttrc for the past few years with great success:

  macro index A ":unset pipe_decode\n|git am -3\n:set pipe_decode\n"
  macro pager A ":unset pipe_decode\n|git am -3\n:set pipe_decode\n"

Basically the raw email message gets piped to "git am" (instead of
patch(1)) so the entire commit message + authorship information goes
into git (I run mutt inside my source trees).  I never have to leave
my email client to apply a patch :)

-- 
Eric Wong