Dima Kuznetsov has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 7: (7 comments) http://gerrit.ovirt.org/#/c/29392/7/lib/vdsm/sigutils.py File lib/vdsm/sigutils.py: Line 28: Line 29: _signal_poller = None Line 30: Line 31: Line 32: def _get_signal_poller(): > Rename it to init(), or register() trivia bit: if user calls register but never actually reads from the read end, python WILL crash once the pipe is full (65536 signals on my laptop) Line 33: ''' Line 34: This function creates a select.poll object that can be used in the same Line 35: manner as signal.pause(). The poll object returns each time a signal was Line 36: received by the process. Line 87: logging.exception( Line 88: 'Received error while reading from pipe') Line 89: except select.error as e: Line 90: if e[0] != errno.EINTR: Line 91: logging.exception('Received error while waiting for signal') > Since we don't care about the timeout (nobody ask for timeout), we can repl I liked this idea when we discussed it at first, but then I realized it still suffers from 'return twice per signal', and there is not way to empty a pipe without potentially sleeping, so I prefer to keep it non-blocking and rely on poll() for waiting. http://gerrit.ovirt.org/#/c/29392/7/tests/sigutilsTests.py File tests/sigutilsTests.py: Line 39: raise e Line 40: else: Line 41: break Line 42: Line 43: def _popen_helper(self, *args): > We don't need to make stuff private in tests. Done Line 44: return subprocess.Popen([CHILD_SCRIPT] + list(args), Line 45: stdout=subprocess.PIPE) Line 46: Line 47: def test_signal_received(self): Line 57: now = time.time() Line 58: self.assertRead(popen.stdout, 'ready\n') Line 59: self.assertRead(popen.stdout, 'done\n') Line 60: later = time.time() Line 61: self.assertTrue((later - now) < TIMEOUT * 3) # 3 is safety factor > Should be TIMEOUT < elapsed < TIMEOUT * 3 Done Line 62: Line 63: def test_signal_10_times(self): Line 64: TIMES = 10 Line 65: popen = self._popen_helper('test_signal_times', str(TIMES)) Line 63: def test_signal_10_times(self): Line 64: TIMES = 10 Line 65: popen = self._popen_helper('test_signal_times', str(TIMES)) Line 66: self.assertRead(popen.stdout, 'ready\n') Line 67: for _ in range(TIMES): > We don't need the loop, 2 times is enough, see the comments in the version Yes but it hardly costs any time (without sleeps). Line 68: time.sleep(0.1) Line 69: popen.send_signal(signal.SIGUSR1) Line 70: self.assertRead(popen.stdout, 'signal\n') Line 71: self.assertRead(popen.stdout, 'done\n') http://gerrit.ovirt.org/#/c/29392/7/tests/sigutilsTests_child.py File tests/sigutilsTests_child.py: Line 24: signal.signal(signal.SIGCHLD, sighandler) Line 25: sys.stdout.write('ready\n') Line 26: func(*args, **kwargs) Line 27: sys.stdout.write('done\n') Line 28: return wrapper > I think this added unnecessary complexity. This is equal to having the triv Done Line 29: Line 30: Line 31: @test_wrapper Line 32: def test_signal_received(): Line 28: return wrapper Line 29: Line 30: Line 31: @test_wrapper Line 32: def test_signal_received(): > Don't name this like a test - this is not a test that testing tool should d Since test code runs simultaneously in 2 processes, I prefer to have two functions with same name to signify what code runs under which test. Line 33: sigutils.wait_for_signal() Line 34: Line 35: Line 36: @test_wrapper -- 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: 7 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
