[
https://issues.apache.org/jira/browse/THRIFT-3781?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15229172#comment-15229172
]
Justin Mills commented on THRIFT-3781:
--------------------------------------
I believe I was wrong in my original claim that {{Thrift::ProtocolException}}
is not a {{StandardError}}. The sources indicate it subclasses from
{{Exception}}, but given the namespacing rules in ruby it will match
{{Thrift::Exception}} first, which _is_ a subclass of {{StandardError}}. So
that bit is a red herring.
I spent a little more time digging into this and less sure that the exception
handling in {{client.rb}} is being bypassed. I monkeypatched some of this to
force it down that path and was still seeing that all subsequent calls to the
server fail. I didn't spend a lot of time on it and I'll try to get back to it
to dig in further, but it appears that there's some data (in my case a ",") in
a buffer somewhere that's being flushed over to the server. I do not believe
it's in the {{HTTPClientTransport}} either, as I implemented a {{#close}}
method on it to clear out it's internal buffer and it appeared to be empty
between requests. Still a leading "," was still written on all subsequent
requests. I suspect it is somewhere in the {{JsonProtocol}} class, but ran out
of time before I could dig in further.
> Thrift::Client can become permanently corrupt when Thrift::ProtocolException
> is raised
> --------------------------------------------------------------------------------------
>
> Key: THRIFT-3781
> URL: https://issues.apache.org/jira/browse/THRIFT-3781
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.9.3
> Reporter: Justin Mills
>
> {{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:
> {code}
> 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'
> {code}
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
