thrift: refactor Thrift router to allow protocol upgrade#4286
Conversation
Modifies the Thrift router to allow protocols to upgrade on initial data from the downstream connection and upgrade an upstream connection before transmitting a request. As part of this work, Transport and Protocol objects are reused across downstream connections and for an upstream's request and response. *Risk Level*: low, upgrade path unused *Testing*: unit tests *Docs Changes*: n/a *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
| } | ||
|
|
||
| /** | ||
| * Check whether a given upstream connection can be upgraded and generates an upgrade request |
There was a problem hiding this comment.
[minor] Checks for consistency with the other method comments
|
|
||
| void initProtocolConverter(ProtocolPtr&& proto, Buffer::Instance& buffer) { | ||
| proto_ = std::move(proto); | ||
| void initProtocolConverter(Protocol* proto, Buffer::Instance& buffer) { |
There was a problem hiding this comment.
The change to a raw pointer here is dangerous.
|
|
||
| Decoder::Decoder(TransportPtr&& transport, ProtocolPtr&& protocol, DecoderCallbacks& callbacks) | ||
| : transport_(std::move(transport)), protocol_(std::move(protocol)), callbacks_(callbacks) {} | ||
| Decoder::Decoder(Transport& transport, Protocol& protocol, DecoderCallbacks& callbacks) |
There was a problem hiding this comment.
With this change from TransportPtr&& transport to Transport& transport, what guarantees that the referenced transport will remain valid for the full life of the Decoder object?
There was a problem hiding this comment.
It's up to the constructor of the Decoder to make sure this is the case. ConnectionManager creates a TransportPtr, ProtocolPtr, and DecoderPtr when it's constructed. The TransportPtr and ProtocolPtr and never replaced so their lifecycle is the duration of the downstream connection.
Each Router has a TransportPtr and ProtocolPtr. A DecoderPtr is held indirectly via ThriftObjectPtr if protocol upgrade occurs for the upstream. Once upgrade is complete that ThriftObjectPtr and DecoderPtr are destroyed. If the request is terminated early the ThriftObjectPtr and DecoderPtr are destroyed before the TransportPtr or ProtocolPtr.
Finally the ConnectionManager::ActiveRpc holds a ResponseDecoder that has references to the Router's TransportPtr and ProtocolPtr. The destruction order there is ResponseDecoder before DecoderFilter so that's ok as well.
brirams
left a comment
brirams
left a comment
There was a problem hiding this comment.
Take my approval with a grain of salt because I'm still ramping up but looks reasonable.
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
(prefer forward reference resolved in the same file to one resolved by a separate file) Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
|
@brian-pane can I get you refresh your review? (5fc9e3a is the only change from before). |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Your "we wish Github supported our permissions model" approval, sir!
4 participants
Modifies the Thrift router to allow protocols to upgrade on initial
data from the downstream connection and upgrade an upstream connection
before transmitting a request. As part of this work, Transport and
Protocol objects are reused across downstream connections and for
an upstream's request and response.
Risk Level: low, upgrade path unused
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io