Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to handle closing sockets? #31

Open
jnicklas opened this issue Jan 30, 2014 · 8 comments
Open

How to handle closing sockets? #31

jnicklas opened this issue Jan 30, 2014 · 8 comments

Comments

@jnicklas
Copy link
Contributor

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
Copy link
Contributor Author

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

@jnicklas
Copy link
Contributor Author

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

@jnicklas
Copy link
Contributor Author

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
Copy link
Member

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

@jnicklas
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

in the kill -9 case it doesn't matter

@jnicklas
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants