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