celluloid / celluloid-zmq

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid actors that talk over the 0MQ protocol
https://celluloid.io
MIT License
83 stars 25 forks source link

How to handle closing sockets? #31

Open jnicklas opened 10 years ago

jnicklas commented 10 years ago

I'm in the process of writing a few specs for this gem. I ran into a problem which I wanted to get some feedback on. In order to separate tests more cleanly from each other, I wrapped all tests in an around filter which calls Celluloid::ZMQ.init before and Celluloid::ZMQ.terminate afterwards, so it sets up a clean ZMQ context for each test. This seems to work nicely, except that zmq_term hangs indefinitely until all sockets created within the context are explicitly closed. This means that every socket we create, we need to explicitly close.

The way I see it, there are two ways of handling this:

1) Simply update the documentation. Leave it up to the user to make sure they call socket.close in a finalizer.

2) Keep track of all sockets created within an actor and add a finalizer to Celluloid::ZMQ which closes them automatically.

While (2) is more user friendly, it could also be problematic, since Celluloid can only have one finalizer per actor (why, btw?), if the user ever adds their own finalizer to the actor, they would overwrite the built in finalizer and mess up the sockets getting closed. So in a way, I think (1) is the better solution at this point.

What do you think?

jnicklas commented 10 years ago

This turns out to be even more insidious. We need to handle closing the waker socket as well.

jnicklas commented 10 years ago

Turns out, fixing the waker issue was trivial. Also sent celluloid/celluloid#376, which is related.

jnicklas commented 10 years ago

It turns out that this is especially problematic if we forcefully kill an actor, since finalizers might not be run, it seems. I added a global registry for sockets in jnicklas/celluloid-jeromq in this commit, because I couldn't get the test suite to even run reliably otherwise.

Even with this change added, I think that people should definitely add finalizers to close sockets to their actors. This is just a last line of defence.

So my suggestion for fixing this issue in other words is to do both (1) and (2) ;). If you agree, I'll port the change from celluloid-jeromq, and I'll update the documentation.

tarcieri commented 10 years ago

@jnicklas you need finalizers to close open sockets. That's the only option

jnicklas commented 10 years ago

@tarcieri I agree. The registry I added in celluloid-jeromq isn't meant as a replacement for closing sockets in finalizers, it's meant as a last line of defence in case the actor thread is forcefully killed via Actor.kill, because this will cause terminate to hang indefinitely, and the app won't exit cleanly.

jnicklas commented 10 years ago

On the other hand, maybe in that case it's warranted that the process needs to be kill -9'd, I'm not sure.

tarcieri commented 10 years ago

in the kill -9 case it doesn't matter

jnicklas commented 10 years ago

I don't follow. I know it doesn't matter when the app is kill -9'd, and for the majority of apps, it won't even matter if we don't terminate cleanly, but it does matter for some apps. For example JRuby applications where the JVM survives the lifetime of the Ruby VM (think TorqueBox).

So we either never call zmq_term or we better make sure that we don't have any sockets still lying around, because in that case, we have to kill -9 the app, because there is no other way it'll exit.

Not calling zmq_term works fine for most apps, and since celluloid-zmq doesn't have an at_exit hook, I imagine that most people don't. But if we do want to call at_exit, we're going to run into this problem.