qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From acon...@apache.org
Subject [1/2] qpid-proton git commit: PROTON-1602: ruby: Possible memory leak in Ruby bindings
Date Fri, 06 Oct 2017 12:51:28 GMT
Repository: qpid-proton
Updated Branches:
  refs/heads/master 446ade386 -> ace1970ef


PROTON-1602: ruby: Possible memory leak in Ruby bindings

Restore Container timeout to avoid excessive CPU and memory use

Fixed test_container tests to be single-threaded.

Ruby library is not yet ready for multi-threaded use, support for
concurrency is in progress.


Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/ace1970e
Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/ace1970e
Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/ace1970e

Branch: refs/heads/master
Commit: ace1970efaea2b5f298acedc62d6838bc424f217
Parents: 121c2d2
Author: Alan Conway <aconway@redhat.com>
Authored: Thu Oct 5 21:00:54 2017 -0400
Committer: Alan Conway <aconway@redhat.com>
Committed: Fri Oct 6 08:51:13 2017 -0400

----------------------------------------------------------------------
 proton-c/bindings/ruby/lib/reactor/reactor.rb  |   1 +
 proton-c/bindings/ruby/tests/test_container.rb | 167 ++++++++------------
 proton-c/bindings/ruby/tests/test_tools.rb     |  86 ++--------
 3 files changed, 85 insertions(+), 169 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/ace1970e/proton-c/bindings/ruby/lib/reactor/reactor.rb
----------------------------------------------------------------------
diff --git a/proton-c/bindings/ruby/lib/reactor/reactor.rb b/proton-c/bindings/ruby/lib/reactor/reactor.rb
index f612876..a84a716 100644
--- a/proton-c/bindings/ruby/lib/reactor/reactor.rb
+++ b/proton-c/bindings/ruby/lib/reactor/reactor.rb
@@ -115,6 +115,7 @@ module Qpid::Proton::Reactor
     end
 
     def run(&block)
+      self.timeout = 3.14159265359
       self.start
       while self.process do
         if block_given?

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/ace1970e/proton-c/bindings/ruby/tests/test_container.rb
----------------------------------------------------------------------
diff --git a/proton-c/bindings/ruby/tests/test_container.rb b/proton-c/bindings/ruby/tests/test_container.rb
index d5b5c9a..9c1d46c 100644
--- a/proton-c/bindings/ruby/tests/test_container.rb
+++ b/proton-c/bindings/ruby/tests/test_container.rb
@@ -26,78 +26,73 @@ URL = Qpid::Proton::URL
 
 class ContainerTest < Minitest::Test
 
-  # Send n messages
-  class SendMessageClient < TestHandler
-    attr_reader :accepted
-
-    def initialize(url, link_name, body)
-      super()
-      @url, @link_name, @message = url, link_name, Message.new(body)
-    end
+  def test_simple()
 
-    def on_start(event)
-      event.container.create_sender(@url, {:name => @link_name})
-    end
+    hc = Class.new(TestServer) do
+      attr_reader :accepted
 
-    def on_sendable(event)
-      if event.sender.credit > 0
-        event.sender.send(@message)
+      def on_start(event)
+        super
+        event.container.create_sender("amqp://#{addr}", {:name => "testlink"})
       end
-    end
 
-    def on_accepted(event)
-      @accepted = event
-      event.connection.close
-    end
-  end
+      def on_sendable(event)
+        if @sent.nil? && event.sender.credit > 0
+          event.sender.send(Message.new("testmessage"))
+          @sent = true
+        end
+      end
 
-  def test_simple()
-    TestServer.new.run do |s|
-      lname = "test-link"
-      body = "hello"
-      c = SendMessageClient.new(s.addr, lname, body).run
-      assert_instance_of(Qpid::Proton::Event::Event, c.accepted)
-      assert_equal(lname, s.links.pop(true).name)
-      assert_equal(body, s.messages.pop(true).body)
+      def on_accepted(event)
+        @accepted = event
+        event.container.stop
+      end
     end
+    h = hc.new
+    Container.new(h).run
+    assert_instance_of(Qpid::Proton::Event::Event, h.accepted)
+    assert_equal "testlink", h.links.first.name
+    assert_equal "testmessage", h.messages.first.body
   end
-
 end
 
 class ContainerSASLTest < Minitest::Test
 
-  # Connect to URL using mechanisms and insecure to configure the transport
-  class SASLClient < TestHandler
+  # Handler for test client/server that sets up server and client SASL options
+  class SASLHandler < TestServer
 
-    def initialize(url, opts={})
-      super()
-      @url, @opts = url, opts
-    end
+    attr_accessor :url
 
-    def on_start(event)
-      event.container.connect(@url, @opts)
+    def initialize(opts={}, mechanisms=nil, insecure=nil, realm=nil)
+      super()
+      @opts, @mechanisms, @insecure, @realm = opts, mechanisms, insecure, realm
     end
 
-    def on_connection_opened(event)
+    def on_start(e)
       super
-      event.container.stop
-    end
-  end
-
-  # Server with SASL settings
-  class SASLServer < TestServer
-    def initialize(mechanisms=nil, insecure=nil, realm=nil)
-      super()
-      @mechanisms, @insecure, @realm = mechanisms, insecure, realm
+      @client = e.container.connect(@url || "amqp://#{addr}", @opts)
+    end
+
+    def on_connection_bound(e)
+      if e.connection != @client # Incoming server connection
+        @listener.close
+        sasl = e.transport.sasl
+        sasl.allow_insecure_mechs = @insecure unless @insecure.nil?
+        sasl.allowed_mechs = @mechanisms unless @mechanisms.nil?
+        # TODO aconway 2017-08-16: need `sasl.realm(@realm)` here for non-default realms.
+        # That reqiures pn_sasl_set_realm() at the C layer - the realm should
+        # be passed to cyrus_sasl_init_server()
+      end
     end
 
-    def on_connection_bound(event)
-      sasl = event.transport.sasl
-      sasl.allow_insecure_mechs = @insecure unless @insecure.nil?
-      sasl.allowed_mechs = @mechanisms unless @mechanisms.nil?
-      # TODO aconway 2017-08-16: need `sasl.realm(@realm)` here for non-default realms.
-      # That reqiures pn_sasl_set_realm() at the C layer - the realm should
-      # be passed to cyrus_sasl_init_server()
+    attr_reader :auth_user
+    def on_connection_opened(e)
+      super
+      if e.connection == @client
+        e.connection.close
+      else
+        @auth_user = e.transport.sasl.user
+      end
     end
   end
 
@@ -119,7 +114,7 @@ class ContainerSASLTest < Minitest::Test
           f.write("
 sasldb_path: #{database}
 mech_list: EXTERNAL DIGEST-MD5 SCRAM-SHA-1 CRAM-MD5 PLAIN ANONYMOUS
-")
+                  ")
         end
         # Tell proton library to use the new configuration
         SASL.config_path(conf_dir)
@@ -140,62 +135,36 @@ mech_list: EXTERNAL DIGEST-MD5 SCRAM-SHA-1 CRAM-MD5 PLAIN ANONYMOUS
   end
 
   def test_sasl_anonymous()
-    SASLServer.new("ANONYMOUS").run do |s|
-      c = SASLClient.new(s.addr, {:sasl_allowed_mechs => "ANONYMOUS"}).run
-      refute_empty(c.connections)
-      refute_empty(s.connections)
-      assert_nil(s.connections.pop(true).user)
-    end
+    s = SASLHandler.new({:sasl_allowed_mechs => "ANONYMOUS"}, "ANONYMOUS")
+    Container.new(s).run
+    assert_nil(s.connections[0].user)
   end
 
   def test_sasl_plain_url()
     # Use default realm with URL, should authenticate with "default_password"
-    SASLServer.new("PLAIN", true).run do |s|
-      c = SASLClient.new("amqp://user:default_password@#{s.addr}",
-                         {:sasl_allowed_mechs => "PLAIN", :sasl_allow_insecure_mechs =>
true}).run
-      refute_empty(c.connections)
-      refute_empty(s.connections)
-      sc = s.connections.pop(true)
-      assert_equal("user", sc.transport.sasl.user)
-    end
+    s = SASLHandler.new({:sasl_allowed_mechs => "PLAIN", :sasl_allow_insecure_mechs =>
true}, "PLAIN", true)
+    s.url = ("amqp://user:default_password@#{s.addr}")
+    Container.new(s).run
+    assert_equal(2, s.connections.size)
+    assert_equal("user", s.auth_user)
   end
 
   def test_sasl_plain_options()
     # Use default realm with connection options, should authenticate with "default_password"
-    SASLServer.new("PLAIN", true).run do |s|
-      c = SASLClient.new(s.addr,
-                         {:user => "user", :password => "default_password",
-                          :sasl_allowed_mechs => "PLAIN", :sasl_allow_insecure_mechs =>
true}).run
-      refute_empty(c.connections)
-      refute_empty(s.connections)
-      sc = s.connections.pop(true)
-      assert_equal("user", sc.transport.sasl.user)
-    end
-  end
-
-  # Test disabled, see on_connection_bound - missing realm support in proton C.
-  def TODO_test_sasl_plain_realm()
-    # Use the non-default proton realm on the server, should authenticate with "password"
-    SASLServer.new("PLAIN", true, "proton").run do |s|
-      c = SASLClient.new("amqp://user:password@#{s.addr}",
-                         {:sasl_allowed_mechs => "PLAIN", :sasl_allow_insecure_mechs =>
true}).run
-      refute_empty(c.connections)
-      refute_empty(s.connections)
-      sc = s.connections.pop(true)
-      assert_equal("user", sc.transport.sasl.user)
-    end
+    opts = {:sasl_allowed_mechs => "PLAIN",:sasl_allow_insecure_mechs => true,
+            :user => 'user', :password => 'default_password' }
+    s = SASLHandler.new(opts, "PLAIN", true)
+    Container.new(s).run
+    assert_equal(2, s.connections.size)
+    assert_equal("user", s.auth_user)
   end
 
   # Ensure we don't allow PLAIN if allow_insecure_mechs = true is not explicitly set
   def test_disallow_insecure()
     # Don't set allow_insecure_mechs, but try to use PLAIN
-    SASLServer.new("PLAIN", nil).run(true) do |s|
-      begin
-        SASLClient.new("amqp://user:password@#{s.addr}",
-                       {:sasl_allowed_mechs => "PLAIN", :sasl_allow_insecure_mechs =>
true}).run
-      rescue TestError => e
-        assert_match(/PN_TRANSPORT_ERROR.*unauthorized-access/, e.to_s)
-      end
-    end
+    s = SASLHandler.new({:sasl_allowed_mechs => "PLAIN", :sasl_allow_insecure_mechs =>
true}, "PLAIN")
+    s.url = "amqp://user:password@#{s.addr}"
+    e = assert_raises(TestError) { Container.new(s).run }
+    assert_match(/PN_TRANSPORT_ERROR.*unauthorized-access/, e.to_s)
   end
 end

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/ace1970e/proton-c/bindings/ruby/tests/test_tools.rb
----------------------------------------------------------------------
diff --git a/proton-c/bindings/ruby/tests/test_tools.rb b/proton-c/bindings/ruby/tests/test_tools.rb
index 2d8f3c6..e9045ee 100644
--- a/proton-c/bindings/ruby/tests/test_tools.rb
+++ b/proton-c/bindings/ruby/tests/test_tools.rb
@@ -49,9 +49,7 @@ class TestPort
     end
   end
 
-  def close
-    @sock.close()
-  end
+  def close() @sock.close(); end
 end
 
 class TestError < Exception; end
@@ -69,27 +67,17 @@ def wait_port(port, timeout=5)
   end
 end
 
-# Handler that creates its own container to run itself, and records some common
-# events that are checked by tests
+# Handler that records some common events that are checked by tests
 class TestHandler < MessagingHandler
 
-  # Record errors and successfully opened endpoints
   attr_reader :errors, :connections, :sessions, :links, :messages
 
   # Pass optional extra handlers and options to the Container
-  def initialize(handlers=[], options={})
+  # @param raise_errors if true raise an exception for error events, if false, store them
in #errors
+  def initialize(raise_errors=true)
     super()
-    # Use Queue so the values can be extracted in a thread-safe way during or after a test.
-    @errors, @connections, @sessions, @links, @messages = (1..5).collect { Queue.new }
-    @container = Container.new([self]+handlers, options)
-  end
-
-  # Run the handlers container, return self.
-  # Raise an exception for server errors unless no_raise is true.
-  def run(no_raise=false)
-    @container.run
-    raise_errors unless no_raise
-    self
+    @raise_errors = raise_errors
+    @errors, @connections, @sessions, @links, @messages = 5.times.collect { [] }
   end
 
   # If the handler has errors, raise a TestError with all the error text
@@ -99,13 +87,13 @@ class TestHandler < MessagingHandler
     while @errors.size > 0
       text << @errors.pop + "\n"
     end
-    raise TestError.new("TestServer has errors:\n #{text}")
+    raise TestError.new("TestHandler has errors:\n #{text}")
   end
 
   # TODO aconway 2017-08-15: implement in MessagingHandler
   def on_error(event, endpoint)
     @errors.push "#{event.type}: #{endpoint.condition.name}: #{endpoint.condition.description}"
-    raise_errors
+    raise_errors if @raise_errors
   end
 
   def on_transport_error(event)
@@ -146,61 +134,19 @@ class TestHandler < MessagingHandler
   end
 end
 
-# A TestHandler that runs itself in a thread and listens on a TestPort
+# A TestHandler that listens on a TestPort
 class TestServer < TestHandler
-  attr_reader :host, :port, :addr
-
-  # Pass optional handlers, options to the container
-  def initialize(handlers=[], options={})
+  def initialize
     super
     @tp = TestPort.new
-    @host, @port, @addr = @tp.host, @tp.port, @tp.addr
-    @listening = false
-    @ready = Queue.new
-  end
-
-  # Start server thread
-  def start(no_raise=false)
-    @thread = Thread.new do
-      begin
-        @container.listen(addr)
-        @container.run
-      rescue TestError
-        ready.push :error
-       rescue => e
-        msg = "TestServer run raised: #{e.message}\n#{e.backtrace.join("\n")}"
-        @errors << msg
-        @ready.push(:error)
-        # TODO aconway 2017-08-22: container.stop - doesn't stop the thread.
-      end
-    end
-    raise_errors unless @ready.pop == :listening or no_raise
-  end
-
-  # Stop server thread
-  def stop(no_raise=false)
-    @container.stop
-    if not @errors.empty?
-      @thread.kill
-    else
-      @thread.join
-    end
-    @tp.close
-    raise_errors unless no_raise
   end
 
-  # start(), execute block with self, stop()
-  def run(no_raise=false)
-    begin
-      start(no_raise)
-      yield self
-    ensure
-      stop(no_raise)
-    end
-  end
+  def host() @tp.host;  end
+  def port() @tp.port;  end
+  def addr() @tp.addr;  end
 
-  def on_start(event)
-    @ready.push :listening
-    @listening = true
+  def on_start(e)
+    super
+    @listener = e.container.listen(addr)
   end
 end


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message