vert-x3 / vertx-proton

Apache License 2.0
26 stars 26 forks source link

Missing exception Handler for receiver #93

Closed johnny closed 5 years ago

johnny commented 5 years ago

Hi, I'm still new to Vertx, so what follows might be a misunderstanding on my part.

What I've seen is, that the ProtonReceiver silently swallows exceptions in the handler. Setting the exceptionHandler on the Vertx instance or the context doesn't work. I would expect to have an exceptionHandler method on the ProtonReceiver.

Example Kotlin code:

class AMQPTTest {
    @Test
    fun start() {
        val vertx = Vertx.vertx()
        vertx.exceptionHandler { e -> System.out.println("An error occured $e") };
        vertx.deployVerticle(Sender())
        vertx.deployVerticle(Receiver())

        Thread.sleep(Duration.ofDays(1).toMillis())
    }
}

class Sender : AbstractVerticle() {
    private val address = "queue://examples"
    override fun start() {
        val client = ProtonClient.create(vertx)

        client.connect("localhost", 5672) { res ->
            if (!res.succeeded()) {
                println("Connect failed: ${res.cause()}")
                return@connect
            }

            val connection = res.result()
            connection.open()

            val sender = connection.createSender(address)
            sender.open()

            vertx.setPeriodic(1000) { _ ->
                if (!sender.sendQueueFull()) {
                    val message = message()
                    message.body = Data(Binary("Ping".toByteArray()))
                } else {
                    println("No credit to send, waiting.")
                }
            }
        }
    }
}

class Receiver : AbstractVerticle() {
    private val address = "examples"

    override fun start() {
        val client = ProtonClient.create(vertx)
        Vertx.currentContext().exceptionHandler { e -> System.out.println("An error occured $e") };

        client.connect("localhost", 5672) { res ->
            if (!res.succeeded()) {
                println("Connect failed: " + res.cause())
                return@connect
            }

            val connection = res.result()
            connection.open()

            val receiver = connection.createReceiver(address)
            receiver.handler { _, msg ->
                val body = msg.body
                val content = when(body) {
                    is AmqpValue -> body.value as String
                    // is Data -> body.value.toString()
                    else -> throw IllegalArgumentException("foobar")
                }
                println("Received message with content: $content")
            }.open()
        }
    }
}

The best solution at the current state I can come up with is to wrap the handler with another handler that handles the exceptions.

gemmellr commented 5 years ago

It is expected that you don't throw from the vertx-proton message/delivery handler. You should handle the delivery by either applying an appropriate disposition (e.g. modified/rejected/released) or having the handler to return as normal and auto-accept (unless configured otherwise), depending on what is appropriate for your particular use case needs.

As to the other exception handlers, I don't see a way for vertx-proton to swallow things so I expect that must be something occurring within vertx-core (where those handlers are defined, along with the networking layer vertx-proton builds on).

gemmellr commented 5 years ago

To follow up, at least the Context exception handler was being fired in 3.5.x in this scenario, so it seems some change in vertx-core prior to 3.6.0 has resulted in that no longer being the case. I don't know if this was a deliberate change or not. Any idea @vietj?

gemmellr commented 5 years ago

Also, as noted on issue 94, this was not silent previously in 3.5.x, with vertx-core logging the exception.

johnny commented 5 years ago

Thanks for following up. I was testing this with 3.6.2. Should have mentioned that :/

calohmn commented 5 years ago

I've had a look at a stacktrace of an exception thrown inside a delivery handler:

 at io.vertx.proton.impl.ProtonReceiverImpl.onDelivery(ProtonReceiverImpl.java:206)
 at io.vertx.proton.impl.ProtonTransport.handleSocketBuffer(ProtonTransport.java:163)
 at io.vertx.core.net.impl.NetSocketImpl$DataMessageHandler.handle(NetSocketImpl.java:392)

 at io.vertx.core.streams.impl.InboundBuffer.handleEvent(InboundBuffer.java:225)     <-- Exception caught here

 at io.vertx.core.streams.impl.InboundBuffer.write(InboundBuffer.java:123)
 at io.vertx.core.net.impl.NetSocketImpl.handleMessage(NetSocketImpl.java:370)
 at io.vertx.core.net.impl.ConnectionBase.handleRead(ConnectionBase.java:386)
 at io.vertx.core.impl.ContextImpl.executeTask(ContextImpl.java:320)
 at io.vertx.core.impl.EventLoopContext.execute(EventLoopContext.java:43)
 at io.vertx.core.impl.ContextImpl.executeFromIO(ContextImpl.java:188)
 at io.vertx.core.net.impl.VertxHandler.channelRead(VertxHandler.java:174)

The exception is caught in InboundBuffer.handleEvent and is handled in InboundBuffer.handleException() - only if an exceptionHandler is set on the InboundBuffer. The latter is not the case here, so the exception is completely ignored.

(ProtonTransport has socket (a io.vertx.core.net.impl.ǸetSocketImpl) has pending (an io.vertx.core.streams.impl.InboundBuffer).)

So it looks to me there is a change needed in Vert.x core, either to pass along a new exceptionHandler defined in ProtonTransport or to call the exceptionHandler from Vertx or the Vertx Context. (EDIT:) provided the exception is (re)thrown from ProtonTransport.handleSocketBuffer (I think it makes sense to handle the exception there first - see https://github.com/vert-x3/vertx-proton/issues/94#issuecomment-461793405).

vietj commented 5 years ago

I think the exception handler on the InboundBuffer should call the NetSocket exception handler when it is set

gemmellr commented 5 years ago

What about when it isnt set, like here? Do what it did in 3.5.x ? Swallow it as it does since 3.6?

vietj commented 5 years ago

I think the most appropriate would be to call ConnectionBase#handleException that will exactly either report the issue to the exception handler or log it.

Julien

On 8 Feb 2019, at 13:34, Robbie Gemmell notifications@github.com wrote:

What about when it isnt set, like here? Do what it did in 3.5.x ? Swallow it as it does since 3.6?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-proton/issues/93#issuecomment-461789048, or mute the thread https://github.com/notifications/unsubscribe-auth/AANxij8BI-8_3WQ6AR0gVyxOZKt_ais_ks5vLW7IgaJpZM4afP_5.

calohmn commented 5 years ago

@gemmellr Any news on this? Should this issue here be re-opened or would the fix be done for #94?