thrift-server icon indicating copy to clipboard operation
thrift-server copied to clipboard

thrift-client-ttwitter-filter: Does not handle upgrade failure or connection state

Open jlai opened this issue 5 years ago • 0 comments

thrift-client-ttwitter-filter appears to incorrectly handle servers that don't support ttwitter (such as the apache node-thrift implementation).

  1. https://github.com/creditkarma/thrift-server/blob/c8e9244a49c8635aa2a7594d8578ac6d201adbbb/packages/thrift-client-ttwitter-filter/src/main/ThriftClientTTwitterFilter.ts#L263 does not handle the thrift EXCEPTION message type which says that __can__finagle__trace__v3__ does not exist, and treats the connection as upgraded even though it shouldn't be.

  2. ttwitter upgrades should be done on a per-connection basis. This filter, however, belongs to the TcpConnection not the underlying connections from the connection pool, so it's very possible that the upgrade request could go to one server and the subsequent ttwitter-prefixed messages could go to other connections that haven't been officially upgraded. In principle, a thrift server with a strong internal state machine could refuse to accept ttwitter-prefixed messages if it hasn't gotten an upgrade request.

jlai avatar Jul 29 '19 21:07 jlai