# Do we need StrageyBase.\_results\_lock? **URL:** \ **Category:** Ansible Developer\ **Created:** [March 19, 2021, 10:41pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814 "2021-03-19T22:41:54Z")\ **Posts on this page:** 7\ **Page:** 1
**Author:** ![moreati](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/moreati/32/5303_2.png) [@moreati](https://forum.ansible.com/u/moreati)\ **Post date:** [March 19, 2021, 10:41pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/1 "2021-03-19T22:41:54Z")
ansible.plugins.strategy.StrategyBase.\_results\_lock is acquired to control access to StrategyBase.\_results and StrategyBase.\_handler\_results. Specifically - in ansible.plugins.strategy.results\_thread\_main() before append() ing a result - in StrategyBase\_process\_pending\_results() before popleft() ing However StrategyBase.\_results & \_handler\_results are both collections.deque objects. From the Python docs > Deques support thread-safe, memory efficient appends and pops from either side of the deque. Thus AFAICT the lock isn’t needed for the deque operations, and could possibly be removed. Have I missed something? Is the lock providing another protection I’ve not noticed? Is it there as a belt and braces measure? If there’s no reason to have it, and interest in removing it, I’ll submit a PR. Alternatively maybe there’s benefit in keeping the lock and using wait()/notify() instead of some of the polling. Regards, Alex ---
**Author:** ![sivel](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/sivel/32/1198_2.png) [@sivel](https://forum.ansible.com/u/sivel)\ **Post date:** [March 22, 2021, 3:38pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/2 "2021-03-22T15:38:23Z")
I did some basic performance testing here, and the lock as currently implemented provides a decent performance improvement. On my machine with 100 hosts, 100 debug tasks, and 25 forks, I found that removing the lock increased execution time by close to 60 seconds. Based on that, I’d be hesitant to remove it. ---
**Author:** ![moreati](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/moreati/32/5303_2.png) [@moreati](https://forum.ansible.com/u/moreati)\ **Post date:** [March 22, 2021, 7:40pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/3 "2021-03-22T19:40:51Z")
Huh, I would not have guessed that. Thanks, I’ll try that next time I can’t explain the presence of some code. ---
**Author:** ![sivel](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/sivel/32/1198_2.png) [@sivel](https://forum.ansible.com/u/sivel)\ **Post date:** [March 22, 2021, 7:42pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/4 "2021-03-22T19:42:52Z")
I’ll honestly say I was surprised too. I expected the opposite. These are questions that chew away at my days; Running endless performance profiling. ---
**Author:** ![moreati](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/moreati/32/5303_2.png) [@moreati](https://forum.ansible.com/u/moreati)\ **Post date:** [March 23, 2021, 9:50pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/5 "2021-03-23T21:50:34Z")
Matt, Could you share your benchmark. I’m seeing barely any difference With lock: ```auto ± hyperfine -- "ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml" **Benchmark #1** : ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml Time ( **mean** ± σ): **6.744 s** ± 0.107 s [User: 27.194 s, System: 1.963 s] Range (min … max): 6.537 s … 6.829 s 10 runs ``` ```auto Without lock $ hyperfine -- "ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml" **Benchmark #1** : ansible-playbook --forks 25 -i 100_hosts.yml 100_debugs.yml Time ( **mean** ± σ): **6.675 s** ± 0.090 s [User: 27.160 s, System: 1.936 s] Range (min … max): 6.499 s … 6.748 s 10 runs ``` $ git rev-parse HEAD c4e211a4291162c17054f44ad5381856ffff802f $ git diff | cat diff --git a/lib/ansible/plugins/strategy/ **init**.py b/lib/ansible/plugins/strategy/ **init**.py index 70dea1ea6f..31e0f58eb5 100644 — a/lib/ansible/plugins/strategy/ **init**.py +++ b/lib/ansible/plugins/strategy/ **init**.py @@ -101,14 +101,13 @@ def results\_thread\_main(strategy): strategy.\_tqm.send\_callback(result.method\_name, \*result.args, \*\*result.kwargs) elif isinstance(result, TaskResult): strategy.normalize\_task\_result(result) - with strategy.\_results\_lock: - # only handlers have the listen attr, so this must be a handler - # we split up the results into two queues here to make sure - # handler and regular result processing don’t cross wires - if ‘listen’ in result.\_task\_fields: - strategy.\_handler\_results.append(result) - else: - strategy.\_results.append(result) - # only handlers have the listen attr, so this must be a handler - # we split up the results into two queues here to make sure - # handler and regular result processing don’t cross wires - if ‘listen’ in result.\_task\_fields: - strategy.\_handler\_results.append(result) - else: - strategy.\_results.append(result) else: display.warning(‘Received an invalid object (%s) in the result queue: %r’ % (type(result), result)) except (IOError, EOFError): @@ -223,7 +222,6 @@ class StrategyBase: self.\_results = deque() self.\_handler\_results = deque() - self.\_results\_lock = threading.Condition(threading.Lock()) # create the result processing thread for reading results in the background self.\_results\_thread = threading.Thread(target=results\_thread\_main, args=(self,)) @@ -524,15 +522,12 @@ class StrategyBase: cur\_pass = 0 while True: try: - self.\_results\_lock.acquire() if do\_handlers: task\_result = self.\_handler\_results.popleft() else: task\_result = self.\_results.popleft() except IndexError: break - finally: - self.\_results\_lock.release() original\_host = task\_result.\_host original\_task = task\_result.\_task $ cat 100\_hosts.yml all: hosts: local[001:100]: vars: ansible\_connection: local $ cat 100\_debugs.yml - hosts: all gather\_facts: false tasks: - debug: loop: “{{ range(100) }}” ---
**Author:** ![sivel](https://sea1.discourse-cdn.com/ansible/user_avatar/forum.ansible.com/sivel/32/1198_2.png) [@sivel](https://forum.ansible.com/u/sivel)\ **Post date:** [March 24, 2021, 2:01pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/6 "2021-03-24T14:01:49Z")
I won’t have time until sometime later, but note that I had 100 actual debug tasks, not a loop of 100 items. ---
**Author:** ![system](https://us1.discourse-cdn.com/ansible/original/1X/1882d9a01da5d8996a862359b0913cc568a39b84.svg) [@system](https://forum.ansible.com/u/system)\ **Post date:** [March 24, 2021, 5:47pm UTC](https://forum.ansible.com/t/do-we-need-strageybase-results-lock/34814/7 "2021-03-24T17:47:47Z")
note that loops are within the same subprocess, while tasks each require their own, so there are very different performance profiles