[THRIFT-3781] Thrift::Client can become permanently corrupt when Thrift::ProtocolException is raised

• Type: Bug
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Affects Version/s: 0.9.3
• Component/s: Ruby - Library
• Assignee: Unassigned
• Reporter: Justin Mills
• Created: 06/Apr/16 13:43
• Updated: 06/Apr/16 21:33

Description

Thrift::Client#send_message_args rescues from StandardError, but Thrift::ProtocolException inherits from Exception. This means if you have an argument to a function that is not valid (ie, missing a required attribute), you can get the following error:

Thrift::ProtocolException: Required field address is unset!
	from ~/.rvm/gems/ruby-2.3.0/gems/my-thrift-gem-1.0/lib/my-thrift-gem/gen-rb/my-thrift-gem_types.rb:32:in `validate'
	from ~/.rvm/gems/ruby-2.3.0/gems/thrift-0.9.3.0/lib/thrift/client.rb:44:in `write'
	from ~/.rvm/gems/ruby-2.3.0/gems/thrift-0.9.3.0/lib/thrift/client.rb:44:in `send_message_args'
	from ~/.rvm/gems/ruby-2.3.0/gems/thrift-0.9.3.0/lib/thrift/client.rb:30:in `send_message'
	from ~/.rvm/gems/ruby-2.3.0/gems/my-thrift-gem-1.0/lib/my-thrift-gem/gen-rb/my_service.rb:22:in `send_example_1'
	from ~/.rvm/gems/ruby-2.3.0/gems/my-thrift-gem-1.0/lib/my-thrift-gem/gen-rb/my_service.rb:17:in `example_1'

Since ProtocolException is not a StandardError, the exception handler is not called, and the @oprot.trans.close line is not executed, leaving the transport with data in it's buffers. All subsequent calls will fail that try to re-use this client because of this data.

It is also possible that this problem only occurs when the validation error happens in the 2nd and beyond arguments to a function, as only then is data likely to have been written.

I can see a couple of potential fixes:

• Make Thrift::ProtocolException inherit from StandardError
• Change the rescue block in client.rb to rescue from all Thrift exceptions that do not inherit from StandardError
• Have the client validate each parameter before attempting to serialize anything, ensuring that the data is more likely to serialize successfully.

From a ruby perspective, I think the first of these is the best fix as I'm not sure a ProtocolException is something that warrants subclassing directly from Exception. It seems more aligned to IOError. Unless the thought here is that ProtocolExceptions are reserved for cases where the client should be abandoned and re-built. If that's the case, I think the last of the options might be best since data validation could be done prior to serialization and need not waste the effort of serialization, nor burn the client connection because of it.