Thriftpy / thriftpy2

Pure python approach of Apache Thrift.
MIT License
572 stars 91 forks source link

Exception not raised correctly when thrift loaded with custom module_name #138

Open JonnoFTW opened 4 years ago

JonnoFTW commented 4 years ago

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?

ethe commented 4 years ago

Thank you, I think it should be fixed.

JonnoFTW commented 4 years ago

@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.