Nir Soffer has posted comments on this change. Change subject: Introducing context manager to terminate async processes on internal fail ......................................................................
Patch Set 1: (7 comments) https://gerrit.ovirt.org/#/c/51407/1/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 675 Line 676 Line 677 Line 678 Line 679 Do we use this ? Line 686 Line 687 Line 688 Line 689 Line 690 This should be: with terminating(proc): finished = proc.wait(cond=stop) if not finished: raise ActionStopped() Line 653: p = AsyncProc(p) Line 654: if data is not None: Line 655: with terminator(p, execCmdLogger): Line 656: p.stdin.write(data) Line 657: p.stdin.flush() We should not use the decorator here, since we want to return the process after writing data. I would use try-except for this special use case. Line 658: Line 659: return p Line 660: Line 661: with terminator(p, execCmdLogger): Line 701: Line 702: return proc.returncode, out, err Line 703: Line 704: Line 705: def terminator(proc, logger): Should be terminating - like running and contextlib.closing Line 706: ''' Line 707: A context manager for taking care for killing process on failure. Line 708: The first exception will be remembered and re-raised. Line 709: Line 704: Line 705: def terminator(proc, logger): Line 706: ''' Line 707: A context manager for taking care for killing process on failure. Line 708: The first exception will be remembered and re-raised. I think this should always terminate the process, not only after errors, like all other context managers (e.g. running will always stop, closing will always close). This is much simpler to use. Line 709: Line 710: Sample usage: Line 711: proc = ExecCmd('cmd', sync=False) Line 712: with terminator(proc, logger) as rollback: Line 713: ... Line 714: proc.communicate() Line 715: ... Line 716: ''' Line 717: def __init__(self, proc, log): We don't need a logger, using standard logger for this. Line 718: self._proc = proc Line 719: self._log = log Line 720: Line 721: def __enter__(self): Line 733: try: Line 734: proc.kill() Line 735: except Exception: Line 736: self._log.error("Failed to kill subprocess") Line 737: six.reraise(*exc_info) It will be much simpler to use a function instead of a class with __enter__ and __exit__: @contextmanager def terminating(proc): try: yield proc finally: if proc.poll() is None: try: proc.kill() except Exception: logging.exception("Cannot kill process") if proc.poll() is None: zombiereapser.addPid(proc.pid) Line 738: Line 739: Line 740: def traceback(on="", msg="Unhandled exception"): Line 741: """ -- To view, visit https://gerrit.ovirt.org/51407 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I003cce39d62dba5644937fafaf2c6e24c526c075 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
