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 <acon...@redhat.com> Authored: Thu Oct 5 21:00:54 2017 -0400 Committer: Alan Conway <acon...@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-unsubscr...@qpid.apache.org For additional commands, e-mail: commits-h...@qpid.apache.org