Split ClientBase to enabled synchronous calls #147
Conversation
| Exception error = null; | ||
| try { | ||
| result = AsyncClientBase.this.invokeRequest(call); | ||
| //TODO a nicer way would be a struct exception base class and only catch that |
There was a problem hiding this comment.
See todo. Do we want something like a StructException as base class for the explicitly declared exceptions which are passed to fail()?
There was a problem hiding this comment.
You're right, in that it would make this bit of code cleaner; I think it might possibly end up complicating other codebases that assume Struct is the base type of Thrifty codegen.
I don't remember exactly what the problem was, but I believe I tried this and couldn't make it work out. I think because I wanted Struct as the absolute base, and that can't work - either it must extend Exception, or we have two parallel hierarchies.
Don't quite recall why the latter didn't work - probably I just didn't want the hassle of having multiple base types.
| ThriftException e = ThriftException.read(protocol); | ||
| protocol.readMessageEnd(); | ||
| handleExceptionMessage(call, e); | ||
| //TODO there was no throw/return here before but the sync impl of handleExceptionMessage throws |
There was a problem hiding this comment.
Not sure about this one. It used to call fail, but not return. Does this mean we can get a proper result or a second fail call afterwards?
There was a problem hiding this comment.
Hm, if it didn't bail out in the old version, that's definitely a bug. A ThriftException usually means that the client and server will never be able to talk successfully to one another!
Throwing in the sync client is the right thing to do; async should finish reading the message and return. Good catch!
There was a problem hiding this comment.
I've changed this to throw a custom exception after handleExceptionMessage(). For sync that will never be reached because the ThriftException is thrown before. The async client will catch the custom exception and just swallow it because it's handleExceptionMessage() already called fail(). (Kotlin's Nothing as return type for handleExceptionMessage() would be really nice here)
We should probably add a test that the async client doesn't call fail() twice after receiving a TMessageType.EXCEPTION. I'm not very familiar with server thrift code, is there an easy way to make it send one?
| */ | ||
| protected void enqueue(MethodCall<?> methodCall) { | ||
| protected final Object execute(MethodCall<?> methodCall) throws Exception { |
There was a problem hiding this comment.
Do we want this here or a second ClientBase subclass like SyncClientBase (in that case handleExceptionMessage() could be abstract in here).
There was a problem hiding this comment.
Mmm, I could go either way, but this seems simpler for now. It would be an easy refactor later, if we find that we need the separation.
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
===========================================
- Coverage 65.57% 65.47% -0.1%
- Complexity 926 933 +7
===========================================
Files 87 88 +1
Lines 4782 4803 +21
Branches 561 562 +1
===========================================
+ Hits 3136 3145 +9
- Misses 1409 1422 +13
+ Partials 237 236 -1
Continue to review full report at Codecov.
|
|
Thanks for the contribution - this looks pretty good! I'd like to take a little bit to review, but have some other work I have to handle first. I'm excited to get this in, so thanks for your patience in the meantime! |
There was a problem hiding this comment.
Nice work, and thanks for your patience.
I don't have much feedback at this point; we'll definitely want conformance tests with synchronous clients, but that will have to wait for codegen support.
Once the todos are addressed/removed, this will be good to go.
| Exception error = null; | ||
| try { | ||
| result = AsyncClientBase.this.invokeRequest(call); | ||
| //TODO a nicer way would be a struct exception base class and only catch that |
There was a problem hiding this comment.
You're right, in that it would make this bit of code cleaner; I think it might possibly end up complicating other codebases that assume Struct is the base type of Thrifty codegen.
I don't remember exactly what the problem was, but I believe I tried this and couldn't make it work out. I think because I wanted Struct as the absolute base, and that can't work - either it must extend Exception, or we have two parallel hierarchies.
Don't quite recall why the latter didn't work - probably I just didn't want the hassle of having multiple base types.
| ThriftException e = ThriftException.read(protocol); | ||
| protocol.readMessageEnd(); | ||
| handleExceptionMessage(call, e); | ||
| //TODO there was no throw/return here before but the sync impl of handleExceptionMessage throws |
There was a problem hiding this comment.
Hm, if it didn't bail out in the old version, that's definitely a bug. A ThriftException usually means that the client and server will never be able to talk successfully to one another!
Throwing in the sync client is the right thing to do; async should finish reading the message and return. Good catch!
| */ | ||
| protected void enqueue(MethodCall<?> methodCall) { | ||
| protected final Object execute(MethodCall<?> methodCall) throws Exception { |
There was a problem hiding this comment.
Mmm, I could go either way, but this seems simpler for now. It would be an easy refactor later, if we find that we need the separation.
|
Awesome, thank you! |
This is my first go at splitting
ClientBasefor #145The second commit contains the actual changes, the other two are just make the diff of the second nicer.