Ananto30/zero

don't swallow Exceptions

Closed this issue · 5 comments

When the server is offline the await zero_client.call("hello_world", None) will just return None - I think it would make more sense to throw an exception. Otherwise one does not know if the server returned None or it was a timeout. And even if None would not be a an allowed return value, one would always have to check

zero/zero/client.py

Lines 90 to 93 in e2c69a9

except Exception as e:
self._socket.close()
self._init_socket()
logging.exception(e)

Exceptions on Server Side also seem to result in just None being returned. I (again) think it would be better to make the failing explicit to the client. Normal HTTP-APIs for example would return an 500 Error Response.

zero/zero/server.py

Lines 264 to 265 in e2c69a9

except Exception as e:
logging.exception(e)

So the idea was the exception should be handled in server and client end, not in the framework itself.

As an example, wrap the server (rpc) function with defined error handler and send specific error_response schema as per your design. So there is a distinction between the actual python client error and error response. wdyt about this @powellnorma ?

When the server is offline the await zero_client.call("hello_world", None) will just return None - I think it would make more sense to throw an exception. Otherwise one does not know if the server returned None or it was a timeout. And even if None would not be a an allowed return value, one would always have to check

Good point! I have added TimeoutException and ConnectionException here #29

So the idea was the exception should be handled in server and client end, not in the framework itself.

But I feel like currently it is handled by the framework - It swallows them and logs them.

I agree that optimally the server would handle these exceptions, but what about bugs that the developer did not think of?

I would argue that this library has to deal with exceptions, since it is the one that calls these functions. So we should ask: what is a good default way of handling exceptions?

And swallowing them and thereby acting to the client as if everything worked just fine (and the function just happened to return None) feels wrong to me. I feel instead being explicit about the failure is more align to what one would expect to happen.

As an example, wrap the server (rpc) function with defined error handler

Yes, but I feel like this would often times cause a lot of boiler plate code. One would have to wrap every server-side function and every client-side function in some way.

What do you think about this PR? #35

Good point! I was trying to mimic what HTTP frameworks do, when you get an error in the server, the server just logs it. But I forgot HTTP has a status code to return something to the client to determine the error. And as we don't have status codes (users can always implement their own way of communicating), we need to show something to the client to let the client know the error happened on the server side. I agree!