Dima Kuznetsov has posted comments on this change. Change subject: signals: Better handle signals to non-main threads ......................................................................
Patch Set 1: (11 comments) http://gerrit.ovirt.org/#/c/29392/1//COMMIT_MSG Commit Message: Line 8: Line 9: There is an issue with using signal.pause() and threads in python. The signal Line 10: might arrive to a non-main thread, and not get processed (because only the Line 11: main thread handles signals) until the main thread receives its own signal. Line 12: The issue is documented and tracked here: > This is related only to functions that block the main thread until it is in You are correct. Line 13: https://bugzilla.redhat.com/show_bug.cgi?id=1114434 Line 14: Line 15: Change-Id: I5dbcd00cec22ef12f2b6253b016dcbd0aa889583 Line 9: There is an issue with using signal.pause() and threads in python. The signal Line 10: might arrive to a non-main thread, and not get processed (because only the Line 11: main thread handles signals) until the main thread receives its own signal. Line 12: The issue is documented and tracked here: Line 13: https://bugzilla.redhat.com/show_bug.cgi?id=1114434 > Also this is not really a python bug, this is how pause works - it does not But maybe it is not how it *should* work in python... Anyway, no harm done in reporting it. Line 14: Line 15: Change-Id: I5dbcd00cec22ef12f2b6253b016dcbd0aa889583 http://gerrit.ovirt.org/#/c/29392/1/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1141: Line 1142: def prependDefer(self, func, *args, **kwargs): Line 1143: self._finally.insert(0, (func, args, kwargs)) Line 1144: Line 1145: > utils.py is becoming little too big - maybe create a new signals module? ok, moving to sigutils.py Line 1146: _signalPoller = None Line 1147: Line 1148: Line 1149: def _getSignalPoller(): Line 1154: Line 1155: This function has to be called from the main thread. Line 1156: ''' Line 1157: if _signalPoller is None: Line 1158: (fd_r, fd_w) = os.pipe() > Lets use nicer names - read_fd, write_fd ? Ok Line 1159: Line 1160: # The write end is going to be written to from a c-level signal Line 1161: # handler, so it is a good idea to make sure it does not block. Line 1162: flags = fcntl.fcntl(fd_w, fcntl.F_GETFL, 0) Line 1157: if _signalPoller is None: Line 1158: (fd_r, fd_w) = os.pipe() Line 1159: Line 1160: # The write end is going to be written to from a c-level signal Line 1161: # handler, so it is a good idea to make sure it does not block. > This is not "a good idea" - this is a requirement of signal.set_wakeup_fd a Ok Line 1162: flags = fcntl.fcntl(fd_w, fcntl.F_GETFL, 0) Line 1163: flags |= os.O_NONBLOCK Line 1164: fcntl.fcntl(fd_w, fcntl.F_SETFL, flags) Line 1165: Line 1162: flags = fcntl.fcntl(fd_w, fcntl.F_GETFL, 0) Line 1163: flags |= os.O_NONBLOCK Line 1164: fcntl.fcntl(fd_w, fcntl.F_SETFL, flags) Line 1165: Line 1166: # This call must be done from the main thread. > Please document in the docstring of this function and waitForSignals that t Docs - Ok. Exception - signal.set_wakeup_fd raises an exception if called from non-main thread, so that is covered. Line 1167: signal.set_wakeup_fd(fd_w) Line 1168: Line 1169: poller = select.poll() Line 1170: poller.register(fd_r, select.POLLIN) Line 1168: Line 1169: poller = select.poll() Line 1170: poller.register(fd_r, select.POLLIN) Line 1171: Line 1172: global _signalPoller > It is common to put global at the start of the function, which makes it cle Ok Line 1173: _signalPoller = poller Line 1174: Line 1175: return _signalPoller Line 1176: Line 1183: ''' Line 1184: poller = _getSignalPoller() Line 1185: try: Line 1186: events = poller.poll() Line 1187: for (fd, flags) in events: > The docs describe poll() as "returning tuple (fd, event) - lets use the ter Ok Line 1188: if flags & select.POLLIN: Line 1189: os.read(fd, 1) Line 1190: except select.error as e: Line 1191: # Signal was received in this thread while poll() was active Line 1185: try: Line 1186: events = poller.poll() Line 1187: for (fd, flags) in events: Line 1188: if flags & select.POLLIN: Line 1189: os.read(fd, 1) > We better read couple of bytes, there is no point in waking up multiple tim ok Line 1190: except select.error as e: Line 1191: # Signal was received in this thread while poll() was active Line 1192: if (e.args[0] == errno.EINTR): Line 1193: return Line 1188: if flags & select.POLLIN: Line 1189: os.read(fd, 1) Line 1190: except select.error as e: Line 1191: # Signal was received in this thread while poll() was active Line 1192: if (e.args[0] == errno.EINTR): > e.args is assuming the structure of the error, while the docs promise that ok Line 1193: return Line 1194: logging.error('Received error while waiting for signal', Line 1191: # Signal was received in this thread while poll() was active Line 1192: if (e.args[0] == errno.EINTR): Line 1193: return Line 1194: logging.error('Received error while waiting for signal', Line 1195: exc_info=True) > Use standard logging.execption instead of non-standard vdsm exc_info. ok -- To view, visit http://gerrit.ovirt.org/29392 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5dbcd00cec22ef12f2b6253b016dcbd0aa889583 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
