thrift
thrift copied to clipboard
Overhaul the rust thrift library
The major changes are:
- Concrete impls of Transport have been removed in favor of a blanket implementation.
- Implement standard error handling idioms, including implementations of std::error::Error
for all error types.
- Various renames to bring things in line with idiomatic rust.
- Change the Protocol trait to use static calls rather than virtual dispatch.
- Allow Protocol's to mutate an internal state by changing the trait methods to receive &mut self.
- Processor::process now receives &self ahead of a multithreaded server implementation.
This can't be landed as-is since there need to be accompanying changes to the code generator, but I wanted to solicit feedback on these changes first. I also have a multithreaded server ready to be PRed based on this branch, which you can see here.
Apparently Github only shows the context for comments on the pull request itself, not the actual commits. That's annoying :/
Thanks for the pull request! Overall these changes look great to me, we'll wait for @maximg to chime in with any other comments.
I'm not sure about the Encode/Decode rename. While Read/Write aren't ideal either, they at least express that data is being written (and possibly sent) on the transport, not just encoded/decoded and waiting to be sent.
@maximg Can you push the benchmark you were using so we can see how changes like these affect performance?
Thanks for the PR! Will go over it this evening. Just committed the benchmark so feel free to compare.
I am looking to make some changes to the code generator to merge these changes as well as add support for more of thrift, notably exceptions. However, I am a novice C++ programmer so it's taking me some time to become familiar with the generator - if either of you has a minute I'd love to chat a little about the code to help me get productive faster.
Feel free to ping me on IRC.
For exceptions could you make sure to check with @gsingh93 - he is working on them already. I'll be happy to chat on skype (maxim.golov)
I've pushed another commit adding a BufferedTransportServer
wrapper which causes the yielded Transport
s to be wrapped in a BufStream
. I think it makes sense to provide this as part of the library since it's a major performance gotcha.
We may even want to integrate BufStream
into the impl of TransportServer
for TcpListener
so the simple thing (passing a TcpListener
as your TransportServer
) does the right thing.
The buffered transport looks good, but I don't think it's necessary to add it to the TcpListener
impl. We're going to eventually follow the same pattern as the other libs and uses a factory (example), which will require the user to pass in a factory instead of a transport. The most basic factory will be a buffered factory, so the user shouldn't run into any surprises with their stream not buffering.
Also, I think TransportServer
should be changed backed to ServerTransport
. This is what the other libraries use, as well as the original thrift whitepaper. It does make sense, as it's the transport the server uses to talk to the client, it isn't the server itself (although both interpretations somewhat make sense).
Ok, I can change it back.