Repository navigation
Add run_command completion callbacks - #428
zxc1031408077 wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #428 +/- ##
==========================================
- Coverage 99.53% 99.47% -0.06%
==========================================
Files 18 18
Lines 1703 1714 +11
==========================================
+ Hits 1695 1705 +10
- Misses 8 9 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pkittenis
left a comment
There was a problem hiding this comment.
Thanks for the interest and PR. Left some comments on handling greenlets spawned as callbacks.
| return HostOutput(host, None, None, None, exception=ex, alias=alias) | ||
| host_out = HostOutput(host, None, None, None, exception=ex, alias=alias) | ||
| if callback is not None: | ||
| spawn(self._run_command_callback, host_out, callback, callback_extra_args) |
There was a problem hiding this comment.
This is not the right approach here. Callback should be executed by wait_finished directly by SSHClient. Parallel client should not need changes other than to pass-through callback and args when calling ParallelSSHClient.run_command. There is no need to spawn more greenlets just for callback.
ParallelClient.run_command -> [many] SSHClient.run_command -> [each SSHClient] run_command + callback
Use HostOutput to keep track of per-host-cmd callbacks, and use them in ParallelSSHClient._join.
| _timeout = read_timeout if read_timeout else timeout | ||
| host_out = self._make_host_output(channel, encoding, _timeout, fully_qualified_command=_command) | ||
| if callback is not None: | ||
| spawn(self._run_command_callback, host_out, callback, callback_extra_args) |
There was a problem hiding this comment.
Should not be spawning callback in greenlet here. This causes issues with race conditions, and is not the right approach.
SSHClient should run callback directly without a greenlet in its wait_finished function, ie after command has finished executing. SSHClient.run_command should not need changes. SSHClient callback should be in wait_finished.
| self.assertEqual(callback_args, [host_output, 'extra']) | ||
|
|
||
| # Give spawned greenlets a chance to finish before the test tears down. | ||
| sleep(0) |
There was a problem hiding this comment.
Should not be necessary, is a race condition. See review comments.
If the callback does something that takes time to finish, eg gevent.sleep(1), a race condition occurs.
Summary
run_commandcallback_extra_argsand invoke the callback after command completionTesting
pytest(24 passed)flake8python -m compileallCloses #275