Nir Soffer has posted comments on this change. Change subject: executor: introduce the executor library ......................................................................
Patch Set 31: (3 comments) http://gerrit.ovirt.org/#/c/29191/31/lib/vdsm/executor.py File lib/vdsm/executor.py: Line 144: self._scheduler = scheduler Line 145: self._busy = False Line 146: self._discarded = False Line 147: _Worker._id += 1 Line 148: name = "Executor-%i" % _Worker._id > if we ever want to have more than one executor, we would need to do a simil Not sure what you sugget, but letting the executor set the name of the workers is probably better. If we have multiple executors each will a name and will name its workers "executor-name-worker-n". And if name is not set we can just use the default thread name. Line 149: self._thread = threading.Thread(target=self._run, name=name) Line 150: self._thread.daemon = True Line 151: self._log.debug('Starting worker %s' % name) Line 152: self._thread.start() Line 154: @property Line 155: def name(self): Line 156: return self._thread.name Line 157: Line 158: def join(self): > we attempt to join only the non-busy workers. why? why should we join() at We join when Executor.stop() is invoke with wait == True. This is the default but if you don't care about joining threads you don't have to join them. If you care about joining we try to join the idle workers, skipping busy workers, so you don't wait forever because a thread is stuck waiting for libvirt. join is racy, and the benefit is questionable, so maybe we should simplify the logic and either join none or all threads. If we change the logic, the default wait should be False, since waiting on possibly stuck workers is not a good default. Line 159: if self._busy: Line 160: raise _WorkerBusy() Line 161: self._log.debug('Waiting for worker %s', self.name) Line 162: self._thread.join() Line 231: except IndexError: Line 232: with self._cond: Line 233: self._waiters += 1 Line 234: if not self._tasks: Line 235: self._cond.wait() > there's a race here if multiple waiting threads have been woken. The spurious wakeup bug is not related to multiple threads, but to wait returning when notify was not called. Since a thread acquire self._cond when returning from wait, we don't care if one or more threads wakes up. They will update the _waiters count safely since only one of the threads can continue, holding self._cond. Can you describe the race? Line 236: self._waiters -= 1 Line 237: Line 238: def clear(self): -- To view, visit http://gerrit.ovirt.org/29191 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic06da1ba57868dc2c7db67a1868ad10087a1cff2 Gerrit-PatchSet: 31 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[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
