sstone/amqp-client

Update Akka to 2.2.0

tiagoboldt opened this issue · 12 comments

Akka needs update to 2.2.0

Hello,
With the latest commits on branch wip-messages the code should be compatible with akka 2.1.4 and 2.2.0
I still have to merge wip-messages into scala2.10, publish 1.3 (with akka 2.1.4), and then publish 1.4 (with akka 2.2.0)
=> if you need amqp-client with akka 2.2.0 right now you can switch to branch wip-messages update pom.xml and build. Of course, if you do so and find something wrong do not hesitate to comment here :-)
=> * very * soon, branch scala2.10 will be based on akka 2.2.0 and I will push snapshots to the sonatype repo
=> and "soon", version 1.4 will be pushed to maven central

RpcSpec is printing a bunch of
java.lang.ClassCastException: com.github.sstone.amqp.RpcClient$Undelivered cannot be cast to com.github.sstone.amqp.RpcClient$Res

I am testing using akka 2.2.0 using sbt.
(Added "com.typesafe.akka" %% "akka-slf4j" % akkaVersion % "test")

Yes, in this test the RpcServer will crash (on purpose) when it receives "5", the route disappears and client messages are returned. I should probably rewrite this test, and anyway the RPC processor that is used is not a very good example. I will push a simple change that will make it more obvious. Thanks!

+1, am looking forward to the akka 2.2.0 version too.

@sstone FYI I tried to upgrade my project from akka 2.1.4 to 2.2.0 using the wip-messages branch, it compiled fine but RabbitMqConnection.createConsumer is not implemented (???) and this broke my tests at runtime. Can you please advise what's your approx. ETA for the akka 2.2.0 version and your plan for RabbitMqConnection, should we keep using this helper or use a different approach? I'll appreciate your thoughts, thanks.

I forked your branch, merged wip-messages into scala2.10, added project/Build.scala so I could import it directly via SBT and created a new tag akka2.2.0-exp1. In case you'd like to see it's at: https://github.com/tomrogers3/amqp-client. Thanks again.

Hi Tom,
I had less time to work on this than I thought, and had to take some time off. I will fix branch wip-messages asap (including the RabbitMqConnection helper), but I can't give you an ETA right now because it seems that many users also use spray (we do at work) and will stick to akka 2.1.4 for some time, so I'm considering publihsing an RC whic targets akka 2.2.0-RC1 first, then akka 2.2.0.
Thanks!

Hi @sstone,
Thanks very much for getting back to me and the updates to wip-messages, these are now working well for my tests with akka 2.2.0, which is great.

FYI I noticed that I'm getting the following warning in my logs after my tests finish running and the system shuts down. As a disclaimer, I'm new to Scala, Akka, RabbitMQ AND your project :), but according to the Akka 2.1.4 and 2.2.0 docs, it seems Akka 2.2.0 provides these akka.log-dead-letters* config options and v2.1.4 didn't. I'm wondering whether perhaps your code was always sending a dead letter and only now is it more visible in >= 2.2.0? It's only a minor issue, the RabbitMQ connections/channels etc and the Actor system still seem to shut down but if you have some thoughts or an approach to fix this in your upcoming versions / RC then that would be great.

/// Log output on shutdown when using Akka 2.2.0
16:17:41 INFO [lt-dispatcher-6] [a.a.RepointableActorRef  ] - Message [com.github.sstone.amqp.ConnectionOwner$Shutdown] from Actor[akka://channel-spec/user/metadata-connection#1226902467] to Actor[akka://channel-spec/user/metadata-connection#1226902467] was not delivered. [1] dead letters encountered. This logging can be turned off or adjusted with configuration settings 'akka.log-dead-letters' and 'akka.log-dead-letters-during-shutdown'.

/// The dead letter
DeadLetter(Shutdown(com.rabbitmq.client.ShutdownSignalException: clean connection shutdown; reason: #method<connection.close>(reply-code=200, reply-text=OK, class-id=0, method-id=0)),Actor[akka://channel-spec/user/metadata-connection#1226902467],Actor[akka://channel-spec/user/metadata-connection#1226902467])

FWIW, I tried to trace through your code in ConnectionOwner.scala to find the issue, I got a little lost with the message passing / FSM flow plus unfortunately for various reasons it's difficult to test/fix this in my environment and fork. Unless you advise otherwise I think the issue may be with the connection shutdown listener you have around lines 160-165. You seem to be sending a Shutdown(cause) message back to the ConnectionOwner actor, but if the actor has already terminated, then is it not already dead and not able to receive this, hence the dead letter? Again, my knowledge of Akka and FSM are limited so I may be wrong and I'm interested in your thoughts.:

        val conn = connFactory.newConnection()
        conn.addShutdownListener(new ShutdownListener {
          def shutdownCompleted(cause: ShutdownSignalException) {
            self ! Shutdown(cause)
          }
        })

Anyway, I hope you may find this of use, thanks again for the updates and all your work on this library, it's otherwise working very well for us.

You're right !: when the connection actor is stopped it closes its connection, the connection calls its shutdown listener, which sends a Shutdown message to the connection actor. Since the call to conn.close() is asynchronous and returns before the connection is actually closed, there's a good chance that the Shutdown message sent at "self ! Shutdown(cause)" will turn up in /deadletters. And as you guessed I didn't see that before the visibility of dead letters was changed... I don't know yet what I'll do about it. Maybe just a warning in README.md ?
Thanks!

Perhaps there are some ideas here (acknowledgements to geidsvig and the akka-amqp project):

https://github.com/geidsvig/akka-amqp/blob/master/src/main/scala/akka/amqp/DurableConnection.scala

I'm not sure if it will make a difference to your library, but perhaps you could try making the ConnectionOwner actor the ShutdownListener, rather than instantiating an anonymous ShutdownListener as you're doing currently - I'm wondering whether that might be enough to keep the actor alive long enough to receive the Shutdown msg. Or if needed, somehow reorder the sequence of events/messages on termination, or add logic to not send the Shutdown msg on system shutdown (why do you need it in that scenario anyway?). Or (a little hacky) move the FSM to the Disconnected state directly rather than sending another Shutdown message? I can't see which line of code is causing the issue nor test a fix here unfortunately but I hope this may help with ideas at least.

Hi all,

I've merged wip-messages with branch scala2.10, which is now compatible with Akka 2.2.0 (i.e. the code is compatible with Akka 2.2.0 but you still have edit the pom file, change akka.version and recompile).

@tomrogers3 I don't think that changing the way the ShutdownListener is instantiated will make a difference, but I don't think that the dead-letter warning is a serious issue and will leave it as is for now. Thanks for taking time to report and investigate this!

See the comments on issue #23: 1.3-SNAPSHOT is built against akka 2.2.1 and there is a snpashot published to the sonatype snapshot repo

I've just published version 1.3-ML1 to maven central