search

celluloid / celluloid-io

UNMAINTAINED: See celluloid/celluloid#779 - Evented sockets for Celluloid actors
https://celluloid.io
MIT License
879 stars 93 forks source link

Only close Selector it not already closed #155

Closed rjattrill closed 8 years ago

rjattrill commented 9 years ago

Refer issue: https://github.com/celluloid/reel/issues/210

digitalextremist commented 9 years ago

Thanks @rjattrill. This fixes your test case, in every case? I re-ran some of the tests that were failing, which are not related to your change. Now it's green.

The code looks good. The next possible failure here is if monitor somehow becomes NilClass or no longer responds to closed? for some other reason... but that's a lot less likely than the issue you're addressing, and beyond the bounds of reasonable behavior. But, there's also the cost of two calls instead of one... in the reactor. The effect on any benchmark for non-shutdown instances is probably negligible.

That has me thinking: at shutdown do we catch this and silence it?

Your way would have the benefit of solving the issue without releasing a new Reel gem, but it is at 0.6.0 but unreleased; it'd be possible to sneak this into there if that ends up being most performant.

rjattrill commented 9 years ago

Yes, @digitalextremist, this patch does fix my issues. Thank you for fixing the failing test cases, I was hoping that they weren't a result of my patch.

Perhaps a combination of both approaches would be warranted. Try using unless monitor.closed? but if we still get exceptions of known type, catch them and clean up the error message going out. Do you think monitor.closed? would ever block?

ioquatix commented 8 years ago

I am going to merge this, it looks good. Further issues can be addressed in another PR.

digitalextremist commented 8 years ago

:+1: ( @rjattrill Celluloid::IO commit! )

rjattrill commented 8 years ago

Thank you @ioquatix and @digitalextremist

ioquatix commented 8 years ago

Awesome, yeah I'll try to work on celluloid-io a bit over the next two weeks so we can do an update. There are also some outstanding PRs on Celluloid which should probably be merged. Good work everyone.