Better exception messages #30

Open
mheffner opened this Issue Jul 7, 2011 · 2 comments

Projects

None yet

3 participants

Contributor
mheffner commented Jul 7, 2011

Not sure which component is at fault here, but inspecting the exception raised to the cassandra gem when an operation fails gives:

#<CassandraThrift::Cassandra::Client::TransportException: CassandraThrift::Cassandra::Client::TransportException>

There does not appear to be any message field. It would be nice to know what exact transport issue led to this exception -- socket error, timeout, server error, etc. Is this information available and simply being lost or is this a problem with the thrift gem?

kkismd commented Nov 11, 2011

I am troubled by this problem, too.
in Rails environment, raised exception's @message and @type is wrong.

 >> begin; Cassandra.new("Test", "noserver:9160").keyspaces; rescue => e; end
 => #<CassandraThrift::Cassandra::Client::TransportException: CassandraThrift::Cassandra::Client::TransportException>
 >> e.instance_variables
 => ["@message", "@type"]
 >> e.instance_variable_get("@message")
 => nil
 >> e.instance_variable_get("@type")
 => "Could not connect to noserver:9160: getaddrinfo: Name or service not known"
 >> e.to_s
 => "CassandraThrift::Cassandra::Client::TransportException"
 >> puts e.backtrace.join("\n");nil
 /home/shimada/src/Rails/reco-git/vendor/gems/thrift-0.7.0/lib/thrift/transport/socket.rb:53:in `open'
 /home/shimada/src/Rails/reco-git/vendor/gems/thrift-0.7.0/lib/thrift/transport/framed_transport.rb:37:in `open'
 /home/shimada/src/Rails/reco-git/vendor/gems/thrift_client-0.7.1/lib/thrift_client/connection/socket.rb:11:in `connect!'
 /home/shimada/src/Rails/reco-git/vendor/gems/thrift_client-0.7.1/lib/thrift_client/abstract_thrift_client.rb:105:in `connect!'
 /home/shimada/src/Rails/reco-git/vendor/gems/thrift_client-0.7.1/lib/thrift_client/abstract_thrift_client.rb:144:in `handled_proxy'
 /home/shimada/src/Rails/reco-git/vendor/gems/thrift_client-0.7.1/lib/thrift_client/abstract_thrift_client.rb:60:in `describe_ring'
 /home/shimada/src/Rails/reco-git/vendor/gems/cassandra-0.12.2/lib/cassandra/cassandra.rb:1059:in `all_nodes'
 /home/shimada/src/Rails/reco-git/vendor/gems/cassandra-0.12.2/lib/cassandra/cassandra.rb:1044:in `reconnect!'
 /home/shimada/src/Rails/reco-git/vendor/gems/cassandra-0.12.2/lib/cassandra/cassandra.rb:1038:in `client'
 /home/shimada/src/Rails/reco-git/vendor/gems/cassandra-0.12.2/lib/cassandra/cassandra.rb:154:in `keyspaces'
 (irb):1:in `irb_binding'
 /home/shimada/.rvm/rubies/ruby-1.8.7-p352/lib/ruby/1.8/irb/workspace.rb:52:in `irb_binding'/home/shimada/.rvm/rubies/ruby-1.8.7-p352/lib/ruby/1.8/irb/workspace.rb:52

create exception instance by hand, it seems to be right.

 >> e = CassandraThrift::Cassandra::Client::TransportException.new(CassandraThrift::Cassandra::Client::TransportException::UNKNOWN, "this is test")
 => #<CassandraThrift::Cassandra::Client::TransportException: this is test>
 >> e.message
 => "this is test"
 >> e.type
 => 0
 >> e.to_s
 => "this is test"

my dev environment:

  • Ruby 1.8.7
  • Rails 2.1.0
  • thrift 0.7.0
  • athrift_client 0.7.1
  • cassandra 0.12.2
Member

The problem here is with the exception wrapping, which assumes the first field in the exception constructor is the message (it's actually the type).

From thrift exceptions.rb:

  class ApplicationException < Exception

    UNKNOWN = 0
    UNKNOWN_METHOD = 1
    INVALID_MESSAGE_TYPE = 2
    WRONG_METHOD_NAME = 3
    BAD_SEQUENCE_ID = 4
    MISSING_RESULT = 5
    INTERNAL_ERROR = 6
    PROTOCOL_ERROR = 7
    INVALID_TRANSFORM = 8
    INVALID_PROTOCOL = 9
    UNSUPPORTED_CLIENT_TYPE = 10

    attr_reader :type

    def initialize(type=UNKNOWN, message=nil)
      super(message)
      @type = type
    end

Note that the second argument is the message, not the first.

In thrift_client abstract_thrift_client.rb:

  def raise_wrapped_error(e, method_name)
    do_callbacks(:on_exception, e, method_name)
    if @options[:wrapped_exception_classes].include?(e.class)
      raise @client_class.const_get(e.class.to_s.split('::').last), e.message, e.backtrace
    else
      raise e
    end
  end

The line raise @client_class.const_get(e.class.to_s.split('::').last), e.message, e.backtrace assumes that the exception accepts a message as its first argument.

The solution to this problem is to fix the exception wrapping to preserve the type and message, though it's not immediately obvious how to do that in a generic way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment