vert-x3 / vertx-tcp-eventbus-bridge

Apache License 2.0
49 stars 44 forks source link

MessageCodec support #25

Closed przxmek closed 4 years ago

przxmek commented 7 years ago

Version

Context

This is an enhancement proposition.

Right now TcpEventBusBridge supports only messages that are JsonObject instances. In case other object (eg. String) is sent exception will occur:

java.lang.ClassCastException: java.lang.String cannot be cast to io.vertx.core.json.JsonObject
    at io.vertx.ext.eventbus.bridge.tcp.impl.TcpEventBusBridgeImpl.lambda$null$4(TcpEventBusBridgeImpl.java:216)
    at io.vertx.core.eventbus.impl.HandlerRegistration.deliver(HandlerRegistration.java:212)
    at io.vertx.core.eventbus.impl.HandlerRegistration.handle(HandlerRegistration.java:191)
    at io.vertx.core.eventbus.impl.EventBusImpl.lambda$deliverToHandler$3(EventBusImpl.java:503)
    at io.vertx.core.impl.ContextImpl.lambda$wrapTask$2(ContextImpl.java:316)
    at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
    at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:418)
    at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:440)
    at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:873)
    at java.lang.Thread.run(Thread.java:745)

While EventBus allows sending different types of objects with help of MessageCodec, TcpEventBusBridge fails to do so.

Proposition

It would be appreciated if sending POJOs with appropriate MessageCodec was possible. But in this case message body should not be treated as a JSON but as a Buffer (that probably would have to be also encoded with e.g. Base64)

lalitjeevanrao commented 6 years ago

Hi @vietj / @pmlopes . Is this issue still open?

vietj commented 6 years ago

I think that the event bus protocol should be able to contain the message codec name, so the protocol should be updated first.

vietj commented 6 years ago

so this is still open

lalitjeevanrao commented 6 years ago

Alright @vietj. I shall look into it and will get back to you if I need some help.

lalitjeevanrao commented 6 years ago

I think that the event bus protocol should be able to contain the message codec name, so the protocol should be updated first.

Hi @vietj , would you suggest message codec name to be in TcpEventBusBridgeImpl or FrameHelper. Or should i create a new class something like DeliveryOptions in EventBus ?

vietj commented 6 years ago

the json messages exchanged over the bus should contain the codec name so the other side can property decode it, @pmlopes WDYT ?

lalitjeevanrao commented 6 years ago

@pmlopes Ping!

niclar commented 4 years ago

@pmlopes, @lalitjeevanrao -This is still very much an issue.

I'm new to virt.x. I just did a proper c++ interface/lib to the tcp bridge sending protobufs etc.

How would one do a workaround to get the cast made and/or the wire object sent. (this should really be supported out of the box though, in my mind)

-In isolation it works if you register a default codec for the cast in question. But in my actual case (& the general case) I can't use RegisterDefaultCodec since the class in question already have another codec default registered that's needed for the "internal" eventbus communication.

Any other ideas ?

Thanks Niclas

pmlopes commented 4 years ago

Hi @niclar long ago we had a discussion during a face to face meeting that we would like to have the underlying protocol to be changed to some standardized format. I've proposed to look into jsonrpc which is a bit similar to what we are doing anyway.

The advantages of using jsonrpc where each node can act as both server and client allows the full requirements of bi directional streaming we support on vertx. Plus as the protocol has got some momentum with visual studio code, some reference implementations have also started to include support for "de facto" support for other codecs.

I've pushed a experimental branch https://github.com/vert-x3/vertx-tcp-eventbus-bridge/tree/experimental/jsonrpc where I started implementing the protocol parser.

So in streaming mode it is compatible with all the libraries used by visual studio code or, plain client<->server jsonrpc.

In order to support other encodings, https://github.com/Microsoft/vs-streamjsonrpc can use content type headers where the payload states which mime type is the message. In their example they have msgpack so adding a protobuf would be also possible and trivial.

We can discuss how to finish the branch or you can help by doing reviews, submitting pull requests, etc...

niclar commented 4 years ago

Great @pmlopes . I can understand that server argument.. (even though I/we'd like that outbound codec header in the old protocol in the meantime..). -I'll try your branch out and see how far it gets me. -Do you think you can get inbound & outbound codec hooks in place for jsonrpc, or is it to far in the future ? I don't have enough time nor all java expertise to do core work on this right now.. but will surely assist in testing, feedback etc.

What functionality would you say is missing currently (primarily in comparison to the old protocol) ?

pmlopes commented 4 years ago

the way codecs work on streaming jsonrpc is by using headers just like on http, so a request:

Content-Type: application/json
Accepts: application/protobuf

PAYLOAD

Could be seen as the inbound is coming as json and the outbound will be protobuf, now the tricky part is that there's no clear way to signal errors if the accepts isn't supported by the current protocol as vscode always replies json so I don't think they ever thought about it. But if a response is like:

Content-Type: application/json

{"error": {"message": "can't encode to application/protobuf"}}

Then it should be OK i guess.

niclar commented 4 years ago

yeah that seems pragmatic enough

niclar commented 4 years ago

FYI , For now, I forked the old lib and added explicit casts to the sendFrame calls to get the outbound code/traffic working.

pmlopes commented 4 years ago

In the meantime I'll try to get this moving forward.