Support thrift exceptions

[maximg]

We need to receive Thrift exceptions and pass them on to the caller as an Err(...). A possible approach is here: http://is.gd/9Ju5q2

[gsingh93]

Currently the generated *Result structs have two fields, success and ouch, where ouch is an Option. Why not just have a single field that's a Result?

[maximg]

ouch should go, it is a stub put in place until this issue is resolved.

[gsingh93]

So I'm going to start working on this. I'm going to make each *Result struct have one field, which will be the value of type T if there's no exception, and Result<T, ExceptionType> if there is an exception.

The only tricky thing is I believe thrift supports throwing more than one exception. We can make the ExceptionType an enum where all the variants are the exception types, but this makes the case when there's one exception type less ergonomic. Should I implement it like this?

[maximg]

Here is the email trace of the discussion we had on this so far:

From: Simon Génier Date: Sun, Feb 1, 2015 at 6:08 PM Subject: Re: rust-thrift: exceptions To: Maxim Golov

The problem with TResult is that the set of exception is different for each method and AFAIK there are no variadic type parameters in Rust.

On Mon Jan 26 2015 at 2:36:43 AM Maxim Golov wrote: One thing we could try to get rid of when adding the exceptions is ProtocolHelpers::receive, I suspect it can be consolidated with "impl Readable" for each struct; also if we pass exceptions through TResult we may drop the need for the ServiceFunctionResult structure altogether.

On Sat, Jan 24, 2015 at 7:02 PM, Simon Génier wrote: Yes, this would work. The upside Is see as opposed to having a flat enum is that there are a lot of functions in the standard lib for working with Result<T>, so the "happy path" where the request is successful will be very easy to consume. The downside is that you have two levels of enum, but I don't think this is an issue.

I'd suggest we go with Result<T> like you suggested and see how it goes.

Simon

On Fri Jan 23 2015 at 3:38:34 PM Maxim Golov wrote: Do you think this approach can help us in any way?

http://is.gd/9Ju5q2

struct BadCastException;
struct OverflowException;

// generated
#[allow(dead_code)]
enum CalcException {
  BadCast(BadCastException),
  Overflow(OverflowException),
  // more
}

#[allow(dead_code)]
enum MyResult<T> {
  Success,
  Error(T)
}

fn calc() -> MyResult<CalcException> {
  MyResult::Error(CalcException::BadCast(BadCastException))
}

fn main() {
  println!("Result: {}", match calc() {
    MyResult::Success => "Ok",
    MyResult::Error(_) => "Error",
  });
}

On Fri, Jan 23, 2015 at 1:54 PM, Simon Génier wrote: Hi,

Since exceptions are listed on a per method method basis, I think the most straightforward way to support that would be to generate an enum such as

enum MyMethodCallResponse { Ok(ReturnValue), Err1(ExceptionType1), Err2(ExceptionType2), ThriftErr(ThriftErr), }

for each service method. I looked at the Haskell implementation for ideas, but even they map Thrift exceptions to the exceptions available in the language, so we on our own here. The tricky part will be to generate good names for the enum members so it is not a pain to use.

[maximg]

So do you mean that we will return a Result<T, Err> where Err would capture both Thrift library exceptions as well as user-defined exceptions?

[gsingh93]

So a method like add in the tutorial would return a TResult<i32>, and a method like calculate would return a TResult<Result<i32, CalculatorException>>, where CalculatorException is defined as:

pub enum CalculatorException {
    InvalidOperation(InvalidOperation)
}

pub struct InvalidOperation {
    what: i32,
    why: String
}

I think that's similar to what you posted above.

[gsingh93]

And this is bikeshedding, but maybe naming it CalculatorErr and naming the variant InvalidOperationErr(InvalidOperation) might be better/more rust-y.

[maximg]

I am fine with your approach but was thinking that maybe we can make it easy to separate processing of successful calls from error handling. At the moment TResult only expects Trift library errors as errors, maybe we can get user-defined exceptions under the same error type. It seems it will not make error handling dramatically more complicated compared to your proposal but will make the sunny path uniform.

One way to do this would be to parameterize ThriftErr on the exception type (note I have not compiled this):

enum ThriftErr<T> {
    ...
    UserErr(T)
}

enum CalculatorErr {
    InvalidOperationErr(invalidOperation),
    SomeOtherErr(SomeType)
}

For methods which do not return an exception we could use (), I'd guess.

BTW, like your suggestion re better rustification, feel free!

[gsingh93]

I think having UserErr as one of the variants of ThriftErr isn't a bad idea, but I think in that case we should clean up ThriftErr. Many of the variants of ThriftErr are errors that the user can't trigger (as far as I know), they're programming errors on our end. BadVersion is an example of this. Let's say a user decides to mach on a ThriftErr to pull out the UserErr. They check the documentation to see what other error variants they should match against. Now they have figure out how each one is triggered and if it's even possible for them to trigger each error to see whether they need to worry about it. If they decide to just pull out the UserErr and ignore the rest with _ => ..., then they might miss important errors like TransportError or InvalidUtf8.

Since you wrote most of this code, do you think you could sort out which errors are relevant for the user and which aren't? Maybe we could group the not relevant ones into an InternalError variant, and the user can just panic if that happens, as that would be a bug in the library.

[maximg]

Agree; it seems some errors overlap and some are not used, and errors from the far end shall all go under the ProtocolError.

Opened #35 for that.

[gsingh93]

I started implementing this, and after removing the ouch field in some of the result structs, I realized the structs were essentially useless. I won't be messing with this while implementing exceptions, but do you think in the future the entire *Result struct could be removed as an optimization, and the direct return values (i.e. i32) could be sent back instead?

[maximg]

Indeed looks like the Result struct (which was more or less directly copied from the C++ design) can be replaced by Rust enums. I am not sure how well it fits the work on exceptions, but as this change will break the client API it will be good to do it before the drop to main. If you decide that it should be done separately, will you open an issue for it?

[gsingh93]

Indeed looks like the Result struct (which was more or less directly copied from the C++ design) can be replaced by Rust enums.

I'm not replacing all of the return values with enums, just their original return types. If something throws an exception, only in that case does it return an enum (the Result enum, specifically). Essentially I'd be changing these lines to send back a Result<i32> instead of a result struct that contains a Result<i32>, and similarly on the receiving side I wouldn't receive a struct containing a Result<i32>, I would receive the Result<i32> directly. That should save some bytes being sent over the network.

Also, I'm not sure how to send a Result<T, E> over the network, which needs to happen regardless of whether we make the above change. It looks like we have support for sending enums, so we can determine whether Ok or Err was sent, but after that we need to read the data stored inside the enum, and as far as I know that's not currently implemented, correct?

[maximg]

Will this solution work with C++ or Java counterpart?

[gsingh93]

Sorry for the delay, I needed some time to think about this.

This won't work with any other server implementations, we have to go the same route as the other libs and declare a field in the Result struct for each type of exception.

We could still salvage our idea of using the Result enum by converting between the Result enum that the user's server code returns/client code receives, and the *Result structs that the server sends to the client (which is transparent to the user). The performance hit of doing this conversion should be small, making the improved ergonomics of this worth it.

I'll try this idea out and see if there are any other issues with this idea. The complicated design of the other libs makes it hard for me to find problems with my ideas without implementing them.

[maximg]

I'd expect that can be handled by the generated code if we do not have to support more than 1 exception per call? The documentation does not seem to state this explicitly, maybe someone on Thrift mailing list or IRC can confirm that?

Do we need to do the conversion when no exception is received? If not, then there should be hardly any performance impact for normal processing.