Be more careful when making the SMinion #34456
Conversation
The SMinion is often used by backends like the salt client and api this change relies more heavily on the pyzmq components to resolve and return the correct loop, and only create the loop if needed.
| if self.opts['transport'] == 'zeromq' and HAS_ZMQ: | ||
| io_loop = zmq.eventloop.ioloop.ZMQIOLoop() | ||
| else: | ||
| io_loop = LOOP_CLASS.current() |
There was a problem hiding this comment.
I know this is already merged. I just wanted to point out that the 4 lines above are functionally equivalent to simply:
io_loop = LOOP_CLASS.current()
The reason is that when HAS_ZMQ is true, LOOP_CLASS is: zmq.eventloop.ioloop.ZMQIOLoop. So even if self.opts['transport'] != 'zeromq', it will use the else case which is functionally the same since LOOP_CLASS is zmq.eventloop.ioloop.ZMQIOLoop. The only real difference in the code from this change and the original code is that you no longer call zmq.eventloop.ioloop.install() in the HAS_ZMQ case. So by simply removing the lines:
if HAS_ZMQ:
zmq.eventloop.ioloop.install()
You would have accomplished the same result, but in a more straight forward way.
There was a problem hiding this comment.
Actually, I just realized what I said isn't 100% true. The first clause of the "if" case doesn't call current(), but the second clause does. So the first clause will create a new IOLoop even if one already exists, and the second will create a new IOLoop only if one doesn't already exist, and use the existing one if it already exists.
My mistake.
There was a problem hiding this comment.
A more clear depiction of this logic would be:
if self.opts['transport'] == 'zeromq':
io_loop = LOOP_CLASS()
else:
io_loop = LOOP_CLASS.current()
Since we know if the transport if ZeroMQ, HAS_ZMQ must be true (otherwise it would fail to load at the transport level). But you are right to question whether or not using current() is appropriate for the other transports. If not, then the logic may go back to a one-liner.
There was a problem hiding this comment.
right, and I think this could be cleaner. The main thing is that the zmq install line seemed to be causing an issue when the loop already exists. So I think we are in a good place, but I do think that we should gate these by transport rather than availability, we don't want to create zmq loop if we have transport set to tcp and just happen to have pyzmq installed
What does this PR do?
When we create the initial loop then the previous code was correct, but if we are running a salt client instance from within an existing loop like from runners, from salt api etc, then we need to not install the zmq interface but rely entirely on pyzmq to create the loop if it does not exist or attach to the loop if it does.
What issues does this PR fix or reference?
Proposed fix for #33697
Tests written?
No
We need to get tests in if this fix does indeed solve the issue