Bug 1065047 - changed exception raised to NodeUnavailableException to in... #5278
Conversation
|
@danmcp - please review |
| @@ -2365,7 +2365,7 @@ def self.rpc_exec(agent, server, force_rediscovery=false, options=rpc_options) | |||
| begin | |||
| result = yield rpc_client | |||
| ensure | |||
| rpc_client.disconnect | |||
| rpc_client.disconnect unless rpc_client.nil? | |||
There was a problem hiding this comment.
I am not seeing how rpc_client can be nil.
There was a problem hiding this comment.
When activeMQ is down and it cannot connect then it is nil, which then causes "undefined method `disconnect' for nil:NilClass" exception to be thrown, which then masks the original exception NodeUnavailableException, which is thrown when connection fails.
There was a problem hiding this comment.
That would mean this line:
rpc_client = rpcclient(agent, flags)
can return nil. Is that the case? Would be a strange pattern. If it does we should catch the issue there and raise the exception immediately. And secondly if it does, this wouldn't be the right fix since rpc_client is referenced several times before it gets to this point.
There was a problem hiding this comment.
This line
rpc_client = rpcclient(agent, flags)
will throw an exception but since ensure gets called regardless of an exception having been thrown earlier in the code it still tries to disconnect.
|
[test] |
|
Online Test Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/test_pull_requests/7396/) |
|
Origin Test Results: Running (https://originci-openshift.rhcloud.com/job/test_pull_requests/2054/) |
… indicate retry advisable (503)
|
Online Merge Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/merge_pull_requests/5278/) (Image: devenv_4687) |
|
Evaluated for online up to c66d9cf |
|
Evaluated for origin up to c66d9cf |
|
[merge] |
Merged by openshift-bot
...dicate retry advisable (503)
Not really dependent but related pull request
https://github.com/openshift/li/pull/2609