Checking the current actor is running not just that it is alive #121
Conversation
Generated by 🚫 danger |
|
Is Is there a way we can write a test for this? Needs a CHANGELOG entry either way. |
|
based on the code they are not even checking the same information. https://github.com/celluloid/celluloid/blob/master/lib/celluloid/task.rb#L133 running? seams to check the status of the job. where alive is just checking if there is a actor. https://github.com/celluloid/celluloid/blob/master/lib/celluloid/mailbox.rb#L107 however if you don't check alive it will raise an error that says celluloid has to be alive. https://github.com/celluloid/celluloid/blob/master/lib/celluloid/proxy/actor.rb#L34 That check is not wrong but just extra insurance to make sure we don't terminate something that can't be terminated. Really it does not harm anything and if anything it is making things much more safe. |
|
Also tested this out on the server that was giving my the connection refused and it no longer gives me that error. Here is some code of a terminate that does a better job of checking if can be terminated. https://github.com/celluloid/celluloid/blob/master/lib/celluloid/task.rb#L108 |
|
As for a test I not total sure if there is a way to make it happen. As the bug that was created was caused by a firewall blocking the network connection making the web socket throw a connection refused error. This would be really trick to make happen in a test but it is not impossible. Will have to spend some time thinking about how to reproduce the bug. |
| @@ -4,6 +4,7 @@ | |||
| * [#116](https://github.com/slack-ruby/slack-ruby-client/pull/116): Use [slack-ruby/slack-api-ref](https://github.com/slack-ruby/slack-api-ref) as machine API reference - [@dblock](https://github.com/dblock). | |||
| * [#116](https://github.com/slack-ruby/slack-ruby-client/pull/116): Added `users_setPhoto` and `users_deletePhoto` to Web API - [@dblock](https://github.com/dblock). | |||
| * [#81](https://github.com/slack-ruby/slack-ruby-client/pull/81): Require faraday 0.9.0 or newer - [@leppert](https://github.com/leppert). | |||
| * [#121](https://github.com/slack-ruby/slack-ruby-client/pull/121): Checking the current actor is running before calling terminate | |||
There was a problem hiding this comment.
Can you please fix the format for this? Needs your name at the end with a period and all ...
The check for the alive should stay as it could be running but not alive which is not likely but possible.
|
I'd like to merge this, it needs to be rebased. Also any ideas about a test @NewDark? Maybe a connection against a server that doesn't exist? |
|
Yes total forgot about this work. Thanks for the reminder. I think that moving work I will have to test into out. I also noticed that there was no tests for this file is there a style guide to follow for writing tests? |
|
Sorry no style guide. There should be tests for everything obviously. |
|
Ok I will make sure to get this all corrected. Been a bit busy but once I get some free time I can have it done. |
|
Bump @NewDark |
|
Sorry I have been a bit over worked with my job @dblock I have not had time to come back to work on this :( |
|
I failed to write a good test for this, so I am going to merge via #137. |
|
Seeing #146, what changed on |
The check for the alive should stay as it could be running but not
alive which is not likely but possible.