Chaining failed future replace source ServiceException failure code by -1

[impolitepanda]

Hi folks,

This issue is created following this thread: https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/vertx/2fG9PHCNDcc/r_UyGMcAAQAJ

I encountered a pretty big problem recently with chaining service calls using the service proxy mechanism which cause information loss in the exception mechanism with the failure code being erased.

As an example, let's say I have 3 services A,B,C which are all using service proxies to be exposed on the event bus. A calls B, B calls C.

Let's now say that if the method call from B to C fails, it returns a failed future containing a ServiceException with a failure code of 404. The proxy mechanism reply to B, in the event bus, with the correct exception. So if I log what's happening in B, it receives, in the handler, a ReplyException with the right failure code. But it's NOT a ServiceException anymore. So when B reply to A, the reply goes in the generated proxy (BVertxProxyHandler class) mechanism where a test is done :

if (res.cause() instanceof ServiceException) {
    msg.reply(res.cause());
} else {
    msg.reply(new ServiceException(-1, res.cause().getMessage()));
}

As it's a ReplyExeption, the failure code is then replaced by -1. And the information is completely lost once it arrives to A.

I think something should me modified in the Vertx proxy handler generation to not put -1 if a failure code is already present in the received exception. Something like that (but cleaner, if possible as I think this is pretty ugly) ?

if (res.cause() instanceof ServiceException) {
    msg.reply(res.cause());
} else if (res.cause() instanceof ReplyException) {
    msg.reply(new ServiceException(((ReplyException) e).failureCode(), res.cause().getMessage()));
} else {
    msg.reply(new ServiceException(-1, res.cause().getMessage()));
}

[slinkydeveloper]

Maybe this one could fix the problem? https://github.com/vert-x3/vertx-service-proxy/pull/90

[impolitepanda]

I am not sure, but I don't think so. If I understand your code correctly, you still impose the "-1" in your "manageFailure" method if the exception is not a ServiceException instance.

[impolitepanda]

But it's true that we could use your "includeDebugInfo" to pass information to the caller and handle it by recreating an exfeption in the caller method. But, to me, that seems to be a pretty devious way to use the debug info of the ServiceException, so I would rather not.

[slinkydeveloper]

I'm not able to reproduce the bug. I tried to do this test and it passes: https://github.com/vert-x3/vertx-service-proxy/commit/fa5961485f2bd3ccf167994cfa8453c37e450502

Can you provide a reproducer?

[impolitepanda]

Hmm... if I understand this test correctly, it probably work because it doesn't go though the event bus ? I will try to make a reproducer from this. It's going to take a bit of time, though :(

[slinkydeveloper]

Please when you create the reproducer use the latest master snapshot, thank you! :smile:

[impolitepanda]

Hmmm... Sorry, but I have a really dumb question. This would be my first time contributing anything to an OSS (if we can even count this as contributing).

When you say "latest master snapshot", what does it mean ? I don't see any tag linking to a snapshot. I don't see any snapshot in maven central that I could link to a commit. And latest commit from master doesn't compile (missing implementation in TestServiceVertxEBProxy). Should I checkout tag 3.7.0 ?

[slinkydeveloper]

First of all, opening an issue is a huge contribution, so thank you for it :smile:! What I mean is enable sonatype snapshot repository (where our CI publishes the snapshots from master branch of each vert-x3 repo) in your pom and use 4.0.0-SNAPSHOT as vertx-codegen version

[impolitepanda]

Ok. So... I created my reproducer. But it doesn't reproduce the problem :'( So sorry, but it's going to take a while longer. I need to find out where the problem really comes from in our production code. Maybe it's not related to the service proxies but its interactions with our exception system in our micro services. I just check and the problem is still occuring. I just need time to find out the real source of the problem.

[impolitepanda]

Ok! Finally managed to reproduce everything! But I cannot find out how to reproduce it using vertxunit. Only when launching the verticles with the vertx Launcher with my IDE.

Here is the link to my repo: https://github.com/impolitepanda/vertx-service-proxy-bug96-reproducer

Would that be sufficient for you to check it out ?

[impolitepanda]

also, sorry, it's in 3.6.3 version, not in 4.0.0-SNAPSHOT yet. I am working on it right now

Done now.

[impolitepanda]

Btw, I was able to trace the problem to the use of the tyoe of Exception I use. We have implemented a MyException class (which extends ServiceException) which contains some stuff for our API and we then extend MyException, with other new types, like NotImplementedException for instance. In Java term, they are all still ServiceException, but, somehow, it doesn't seem to work with service proxies. If you change the line Service96ReproducerImpl:67 from my example to use a ServiceException, it works as intended.

I really don't understand what's the problem here.

[slinkydeveloper]

Sorry for the delay, I'm looking at your reproducer now

[slinkydeveloper]

So, as far as i know, the problem happens only when in clustered?

[slinkydeveloper]

Ok I got the problem. When you're services are on your local JVM, the event bus just sends the exception as is. When you work in a clustered environment, the event bus does an encoding/decoding phase using some registered MessageCodec like this one https://github.com/vert-x3/vertx-service-proxy/blob/master/src/main/java/io/vertx/serviceproxy/ServiceExceptionMessageCodec.java The problem here is that you don't have any registered codec for your exceptions, so the event bus does the best he can serializing/deserializing it with ReplyException message codec. Please try to create MessageCodec for your exceptions, register in all verticles using vertx.eventBus().registerDefaultCodec(NotImplementedException.class, new NotImplementedExceptionMessageCodec()); and it should work like a charm

[impolitepanda]

OK! I'll try that! I was looking at codecs before opening the issue and google group post but as my exception all extended ServiceException, I thought it would be using the ServiceExceptionMessageCodec. I didn't think it didn't work with inherited classes (and didn't double check). I'll keep you informed as soon as my tests are concluded with your proposition :)

[impolitepanda]

Ok, I tested your suggestion. It indeed works like a charm :) Do you think it would be feasible to modify the codec mechanism for it to work with inheritance? (just to avoid having to create n message codecs which are actually going to be the exact same) I have difficulty reading the core source code for vertx (plus, my pov/experience is only java based so I don't know what would be the impacts on other supported languages)

[slinkydeveloper]

I don't think is something simple to achieve, because the MessageCodec are designed to work with binary data and the matching is done through Class equals. I suggest you to use includeDebugInfo flag and throw every ServiceException with a cause, it serialize the cause type and message in json

[impolitepanda]

Ok, i will check if that does the job for us. In any case, thanks a lot for your time and great help! I am just sorry I didn't look deeper in the codecs when I started :) At least I now have a better knowledge of how this works !