thriftpy2 icon indicating copy to clipboard operation
thriftpy2 copied to clipboard

Exception not raised correctly when thrift loaded with custom module_name

Open JonnoFTW opened this issue 4 years ago • 2 comments

I have an issue when trying to raise and catch the exact exception from an included file. My thrift is:

#TestSpect.thrift
include "errors.thrift"
service TestService {
    void do_error() throws (1: errors.TestException e)
}
#errors.thrift
exception TestException {
    1: string message
}

The following pytest code succeeds:

def test_thrift_exception():
    import thriftpy2
    from thriftpy2.rpc import make_client, make_server
    from multiprocessing import Process
    from time import sleep
    test_thrift = thriftpy2.load("TestSpec.thrift", module_name="test_thrift")
    test_errors_thrift = thriftpy2.load("errors.thrift")

    class TestHandler:
        def do_error(self):
            raise test_errors_thrift.TestException("Error thrown!")

    def run_server():
        server = make_server(test_thrift.TestService, TestHandler())
        server.serve()
    p = Process(target=run_server,)
    p.start()
    sleep(0.5)
    try:
        client = make_client(test_thrift.TestService)

        with pytest.raises(test_errors_thrift.TestException) as e:
            client.do_error()
    finally:
        p.kill()

Changing test_errors_thrift to:

test_errors_thrift = thriftpy2.load("errors.thrift", module_name="test_errors_thrift")

Causes the test to fail. I suspect this is because test_thrift.TestService.do_error_result.thrift_spec is:

{1: (12, 'e', <class 'errors.TestException'>, False)}

But the error raised is a:

test_errors_thrift.TestException

So it can't actually raise the error correctly because it can't be put into the error result because:

isinstance(test_errors_thrift.TestException, errors.TestException)

returns False, per TProcessor.handle_exception from: https://github.com/Thriftpy/thriftpy2/blob/a8b7873078a6f38a2f052964d439ab888e32fcf4/thriftpy2/thrift.py#L303-L312

Would the correct fix here be to ensure that generated thrift_specs use the custom module name?

JonnoFTW avatar Sep 24 '20 01:09 JonnoFTW

Thank you, I think it should be fixed.

ethe avatar Sep 24 '20 03:09 ethe

@ethe after digging around in the code, it looks like these thrift_specs are being generated in _make_service here:

https://github.com/Thriftpy/thriftpy2/blob/a8b7873078a6f38a2f052964d439ab888e32fcf4/thriftpy2/parser/parser.py#L898-L905

Which comes from funcs which are a ply.yacc.YaccProduction which are generated by ply.

At this point I can only only see some kind of monkey-patching fix to replace the module instance after the service class is generated. Maybe there's some other way to do it but I'm not sure.

JonnoFTW avatar Sep 24 '20 03:09 JonnoFTW