circus-tent / circus

A Process & Socket Manager built with zmq
http://circus.readthedocs.org/
Other
1.55k stars 258 forks source link

Error parsing args when use_papa = True #930

Closed cgarciaarano closed 8 years ago

cgarciaarano commented 8 years ago

Hi,

we've found that Circus 0.12.2 is not parsing args correctly when use_papa = True. With this config file:

[circus]
check_delay = 5
endpoint = tcp://127.0.0.1:5555
pubsub_endpoint = tcp://127.0.0.1:5556
statsd = true

[watcher:app1]
cmd = ping
args = 8.8.8.8
use_papa=True

The debug log shows:

2015-09-16 19:35:57 circus[17841] [DEBUG] cmd: ping
2015-09-16 19:35:57 circus[17841] [DEBUG] args: ['8', '.', '8', '.', '8', '.', '8']
2015-09-16 19:35:57 circus[17841] [DEBUG] process args: ['ping', '8', '.', '8', '.', '8', '.', '8']

and the process keeps flapping.

Commenting out in papa_process_proxy.py the self.args assign:

    def spawn(self):
        # noinspection PyUnusedLocal
        socket_names = set(socket_name.lower()
                           for socket_name in self.watcher._get_sockets_fds())
        self.cmd = self._fix_socket_name(self.cmd, socket_names)
        #self.args = [self._fix_socket_name(arg, socket_names)
        #             for arg in self.args]
        args = self.format_args()
        stdout = _bools_to_papa_out(self.pipe_stdout, self.close_child_stdout)
        stderr = _bools_to_papa_out(self.pipe_stderr, self.close_child_stderr)

seems to fix this behaviour, because args is being processed as a string, not a list. I'm not really sure what _fix_socket_name() is doing. Maybe this fix is enough?

self.args = [self._fix_socket_name(arg, socket_names)
                     for arg in self.args.split(' ')]

Regards

ayashjorden commented 8 years ago

Hi, @k4nar, I'm having the same issue with version 0.12.1 . Any time frame for the fix?

Thank you, Yarden

k4nar commented 8 years ago

@scottkmaxwell : Can I assign you to this one ?

cgarciaarano commented 8 years ago

I can pull request my proposal, but I'm not really sure it's correct, it could have side effects.

k4nar commented 8 years ago

We are already splitting the args at some point, so I think that self._fix_socket_name should be applied at this point.

scottkmaxwell commented 8 years ago

Yeah, it sounds like I was expecting args to already be a list at this point. I am completely swamped at the moment but I can try to have a look toward the end of the week if somebody hasn't figured out a proper fix yet.

ayashjorden commented 8 years ago

@scottkmaxwell any update here?

scottkmaxwell commented 8 years ago

Sorry this took so long.

ayashjorden commented 8 years ago

@k4nar when can we expect a release containing this fix?

k4nar commented 8 years ago

@ayashjorden : Sorry for the delay, I'll try to make a new release this week.

ayashjorden commented 8 years ago

Thank you @k4nar :)

antgel commented 8 years ago

Hey all, has the fix been released yet?

k4nar commented 8 years ago

Sorry, I've been experiencing some troubles with our tests suite (cf #984 for progress), and I would really like to fix that before releasing a new version. I'll push as much effort as I can to make it happen this week.

k4nar commented 8 years ago

I've merged #984. I'm giving some time to the opened PRs to be rebased & merged, and I'll release a new version.

ayashjorden commented 8 years ago

Thank you for the update, looking forward to it :)

On Tue, Jul 5, 2016 at 2:13 PM, Yannick PÉROUX notifications@github.com wrote:

I've merged #984 https://github.com/circus-tent/circus/pull/984. I'm giving some time to the opened PRs to be rebased & merged, and I'll release a new version.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/circus-tent/circus/issues/930#issuecomment-230451099, or mute the thread https://github.com/notifications/unsubscribe/AEAnU2IKjlNl0Rdss-09wEQlogZ5ltM8ks5qSjxQgaJpZM4F-l60 .

k4nar commented 8 years ago

@ayashjorden : I just released Circus 0.14. Sorry for the (very) long delay!

ayashjorden commented 8 years ago

Thank you! @k4nar