Change in vdsm[master]: signals: Handle signals to non-main threads
dkuznets at redhat.com
dkuznets at redhat.com
Fri Aug 1 16:19:13 UTC 2014
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 <dkuznets at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list