search

ruby-amqp / bunny

Bunny is a popular, easy to use, mature Ruby client for RabbitMQ
Other
1.39k stars 303 forks source link

Can't rescue from OpenSSL::SSL::SSLError #583

Closed carlhoerberg closed 5 years ago

carlhoerberg commented 5 years ago

While connected over AMQPS, and Thread.abort_on_exception = true, I sometimes get this, and the whole apps crashes: 

#<Thread:0x000000000254bb48@/app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:35 run> terminated with exception (report_on_exception is true):
/app/vendor/ruby-2.6.3/lib/ruby/2.6.0/openssl/buffering.rb:182:in `sysread_nonblock': SSL_read: decryption failed or bad record mac (OpenSSL::SSL::SSLError)
from /app/vendor/ruby-2.6.3/lib/ruby/2.6.0/openssl/buffering.rb:182:in `read_nonblock'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/cruby/ssl_socket.rb:45:in `block in read_fully'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/cruby/ssl_socket.rb:44:in `loop'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/cruby/ssl_socket.rb:44:in `read_fully'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/transport.rb:239:in `read_fully'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/transport.rb:261:in `read_next_frame'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:73:in `run_once'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:39:in `block in run_loop'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:36:in `loop'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:36:in `run_loop'
/app/vendor/ruby-2.6.3/lib/ruby/2.6.0/openssl/buffering.rb:182:in `sysread_nonblock': SSL_read: decryption failed or bad record mac (OpenSSL::SSL::SSLError)
from /app/vendor/ruby-2.6.3/lib/ruby/2.6.0/openssl/buffering.rb:182:in `read_nonblock'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/cruby/ssl_socket.rb:45:in `block in read_fully'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/cruby/ssl_socket.rb:44:in `loop'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/cruby/ssl_socket.rb:44:in `read_fully'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/transport.rb:239:in `read_fully'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/transport.rb:261:in `read_next_frame'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:73:in `run_once'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:39:in `block in run_loop'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:36:in `loop'
from /app/vendor/bundle/ruby/2.6.0/gems/bunny-2.14.2/lib/bunny/reader_loop.rb:36:in `run_loop'

Can we make this more graceful? Worst case just set Thread.current.abort_on_exception = false in the reader_loop thread?

michaelklishin commented 5 years ago

OpenSSL::SSL::SSLError is a descendant of StandardError and the loop handles Exception, which is the base exception class StandardError is a descendant of.

I don't see where Bunny sets Thread#abort_on_exception for the reader loop thread. Consumer work pool has a setting for that.

michaelklishin commented 5 years ago

@carlhoerberg I guess what I'm saying is, I don't see why the exception would not be caught by t he existing code. Bunny does not set Thread#abort_on_exception for the I/O loop thread. I'd accept a PR that makes this an optional feature that defaults to false but this would be a workaround. We don't really know what the root cause is.

michaelklishin commented 5 years ago

Doh, the very first line says you globally set Thread.abort_on_exception. Bunny should not be concerned with such global settings; their users should understand the risks: exceptions will bee propagated to the main thread.

I found no way to "out out of" the global Thread.abort_on_exception = true without subclassing Thread since in Ruby, Thread.new begins callable execution immediately:

Thread.abort_on_exception = true
t = Thread.new { raise "oops" }
t.abort_on_exception = false

does not opt the thread out. Moreover,

t = Thread.new { sleep 5; raise "oops" }
t.abort_on_exception = false
Thread.abort_on_exception = true

suggests that the class flag overrides any previously set Thread instance values.

Subclassing Thread is way out of scope for Bunny.

carlhoerberg commented 5 years ago

Yeah, let's ignore abort_on_exception and fix the root cause instead.

carlhoerberg commented 5 years ago

Opened the issue late last night, didn't even look at the code, just opened the issue to remember it in the morning. Submitted a PR for it now. Sorry for causing confusion with Thread.abort_on_exception.

michaelklishin commented 5 years ago

2.14.3 is out.