Fix race condition in threadpool shrink code. #204

the-allanc · 2019-05-22T20:33:02Z

❓ What kind of change does this PR introduce?

❓ What is the current behavior? (You can also link to an open issue here)

This was previously reported as a CherryPy bug.

📋 Other information:

There was a previously submitted fix in the original ticket. I've transplanted the idea behind it into cheroot, and changed the counter to a deque object instead (as I suspect modifying a counter on an object is not an atomic operation).

📋 Checklist:

I think the code is well written
I wrote good commit messages
I have squashed related commits together after the changes have been approved
Unit tests for the changes exist
Integration tests for the changes exist (if applicable)
I used the same coding conventions as the rest of the project
The new code doesn't generate linter offenses
Documentation reflects the changes
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences

This change is

the-allanc · 2019-05-22T22:02:06Z

To test this, I've been using a basic CherryPy app (with cherrypy_dynpool installed):

import cherrypy
import cherrypy_dynpool
import time

class HelloWorld(object):
    @cherrypy.expose
    def index(self):
        time.sleep(0.5)
        return "Yo World!"


def main():
    cherrypy.engine.threadpool_monitor = cherrypy_dynpool.ThreadPoolMonitor(cherrypy.engine)
    cherrypy.engine.threadpool_monitor.subscribe()

    cherrypy.config.update({
        'server.thread_pool': 5,
        'server.thread_pool_max': 600,
        'server.thread_pool_minspare': 2,
        'server.thread_pool_maxspare': 40,
        'server.thread_pool_frequency': 4,
        'server.thread_pool_shrink_frequency': 10,
        'server.thread_pool_log_frequency': 3,
    })

    app = cherrypy.tree.mount(HelloWorld(), '/', config={})
    cherrypy.engine.start()
    cherrypy.engine.threadpool_monitor.configure(
        thread_pool=cherrypy.server.httpserver.requests,
        logger=cherrypy.engine.log
    )
    cherrypy.engine.block()


if __name__ == '__main__':
    main()

I then use ApacheBench to fire multiple requests at the server: ab -n 6000 -c 300 http://localhost:8080/

On my machine, both versions of cheroot (before and after my changes) fail to fulfill all 6000 requests (they normally manage around 5800 requests). But in the code before my changes, the logs look like this at the end:

[22/May/2019:22:53:34] ENGINE Thread pool: [current=64/idle=64/queue=0]
[22/May/2019:22:53:41] ENGINE Thread pool: [current=64/idle=64/queue=0] Shrinking by 59
[22/May/2019:22:53:43] ENGINE Thread pool: [current=33/idle=33/queue=23]
[22/May/2019:22:55:10] ENGINE Thread pool: [current=33/idle=33/queue=112]

This is the result of ApacheBench finishing running, and then cheroot being in a state where threads are apparently idle, but they are no longer handling any requests. The last line is me attempting to re-run ApacheBench against the same server, but no requests are being processed at all.

Repeating that against cheroot with my changes in place:

[22/May/2019:22:59:31] ENGINE Thread pool: [current=41/idle=41/queue=0] Shrinking by 36
[22/May/2019:22:59:32] ENGINE Thread pool: [current=41/idle=5/queue=0]
[22/May/2019:22:59:41] ENGINE Thread pool: [current=41/idle=5/queue=0] Shrinking by 2
[22/May/2019:22:59:41] ENGINE Thread pool: [current=21/idle=5/queue=0]
[22/May/2019:22:59:51] ENGINE Thread pool: [current=21/idle=5/queue=0] Shrinking by 2
[22/May/2019:22:59:53] ENGINE Thread pool: [current=12/idle=5/queue=0]
[22/May/2019:23:00:01] ENGINE Thread pool: [current=12/idle=5/queue=0] Shrinking by 2
[22/May/2019:23:00:02] ENGINE Thread pool: [current=7/idle=5/queue=0]
[22/May/2019:23:00:11] ENGINE Thread pool: [current=7/idle=5/queue=0] Shrinking by 2
[22/May/2019:23:00:11] ENGINE Thread pool: [current=5/idle=5/queue=0]

The thread pool successfully shrinks down to the desired size, and you can re-run ApacheBench against the server too. Note that there's nothing stuck in the queue, either.

cheroot/workers/threadpool.py

webknjaz · 2019-05-24T20:26:15Z

cheroot/workers/threadpool.py

@@ -223,6 +234,7 @@ def shrink(self, amount):
        # to remove. As each request is processed by a worker, that worker
        # will terminate and be culled from the list.
        for n in range(n_to_remove):
+            self._pending_shutdowns.append(None)


You probably wanted to use the constant here:

Suggested change

self._pending_shutdowns.append(None)

self._pending_shutdowns.append(_SHUTDOWNREQUEST)

No, the object we store in _pending_shutdowns is irrelevant. That constant is important in the requests queue.

Maybe have another named constant for this? I don't like it being a magic value with no meaning.

@the-allanc WDYT?

It's not a magic value, it's a meaningless value. Naming a constant for it and then not referring to that named constant anywhere else would be more confusing.

I needed something to act as a thread-safe counter, and using a deque seemed to be the most straight-forward option.

cheroot/workers/threadpool.py

webknjaz · 2019-05-24T20:33:02Z

@the-allanc plz sync this branch with master: i've fixed linter errors there

webknjaz · 2019-05-24T20:34:41Z

I've manually triggered canceled stages in travis

codecov · 2019-06-25T17:42:21Z

Codecov Report

Merging #204 into master will decrease coverage by 2.06%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage   74.91%   72.84%   -2.07%     
==========================================
  Files          23       23              
  Lines        3580     4346     +766     
==========================================
+ Hits         2682     3166     +484     
- Misses        898     1180     +282

the-allanc · 2019-06-25T17:49:05Z

@the-allanc plz sync this branch with master: i've fixed linter errors there

Done.

webknjaz · 2019-06-25T18:15:18Z

Apparently, this PR somehow breaks tests under Python 2.7 under Windows.

webknjaz · 2019-06-25T18:16:41Z

@the-allanc In general, I have no objections except we need AppVeyor CI to be green first.

cheroot/workers/threadpool.py

jaraco · 2019-09-02T17:15:59Z

This looks good to me. I don't know when I'll have time to give it proper attention, so happy to move forward once the tests are stable/addressed.

jaraco

LGTM

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

webknjaz · 2019-09-23T21:56:44Z

cheroot/workers/threadpool.py

+        # Remove any dead threads from our list
+        for t in [t for t in self._threads if not t.is_alive()]:
+            self._threads.remove(t)
+            try:


This is a perfect case for contextlib usage

Suggested change

try:

with contextlib.suppress(IndexError):

Fine with me.

webknjaz · 2019-09-25T23:55:00Z

I've fixed AppVeyor on master so it's probably worth rebasing this PR.

webknjaz reviewed May 24, 2019

View reviewed changes

cheroot/workers/threadpool.py Outdated Show resolved Hide resolved

webknjaz reviewed May 24, 2019

View reviewed changes

cheroot/workers/threadpool.py Outdated Show resolved Hide resolved

webknjaz reviewed May 24, 2019

View reviewed changes

cheroot/workers/threadpool.py Show resolved Hide resolved

webknjaz reviewed May 24, 2019

View reviewed changes

cheroot/workers/threadpool.py Show resolved Hide resolved

the-allanc added 2 commits June 25, 2019 18:31

Fix race condition in threadpool shrink code.

ee8b43b

More compact code in "clear dead threads" method.

7458d7d

webknjaz reviewed Jun 25, 2019

View reviewed changes

cheroot/workers/threadpool.py Outdated Show resolved Hide resolved

webknjaz requested a review from jaraco June 25, 2019 18:23

webknjaz added bug Something is broken enhancement Improvement labels Jun 25, 2019

jaraco approved these changes Sep 11, 2019

View reviewed changes

Use Thread.is_alive, not Thread.isAlive.

b37fd02

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

webknjaz reviewed Sep 23, 2019

View reviewed changes

jaraco added 2 commits September 25, 2019 22:21

Merge branch 'master' into thread-shrink-race-condition

92e80c8

Update changelog

347c572

jaraco merged commit a6f716a into cherrypy:master Sep 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition in threadpool shrink code. #204

Fix race condition in threadpool shrink code. #204

the-allanc commented May 22, 2019 •

edited by webknjaz

the-allanc commented May 22, 2019 •

edited by webknjaz

webknjaz May 24, 2019

the-allanc Jun 25, 2019

webknjaz Jun 25, 2019

webknjaz Sep 23, 2019

the-allanc Sep 30, 2019

webknjaz commented May 24, 2019

webknjaz commented May 24, 2019

codecov bot commented Jun 25, 2019 •

edited

the-allanc commented Jun 25, 2019 •

edited by webknjaz

webknjaz commented Jun 25, 2019

webknjaz commented Jun 25, 2019

jaraco commented Sep 2, 2019

jaraco left a comment

webknjaz Sep 23, 2019

the-allanc Sep 30, 2019

webknjaz commented Sep 25, 2019

	self._pending_shutdowns.append(None)
	self._pending_shutdowns.append(_SHUTDOWNREQUEST)

Fix race condition in threadpool shrink code. #204

Fix race condition in threadpool shrink code. #204

Conversation

the-allanc commented May 22, 2019 • edited by webknjaz

the-allanc commented May 22, 2019 • edited by webknjaz

webknjaz May 24, 2019

Choose a reason for hiding this comment

the-allanc Jun 25, 2019

Choose a reason for hiding this comment

webknjaz Jun 25, 2019

Choose a reason for hiding this comment

webknjaz Sep 23, 2019

Choose a reason for hiding this comment

the-allanc Sep 30, 2019

Choose a reason for hiding this comment

webknjaz commented May 24, 2019

webknjaz commented May 24, 2019

codecov bot commented Jun 25, 2019 • edited

Codecov Report

the-allanc commented Jun 25, 2019 • edited by webknjaz

webknjaz commented Jun 25, 2019

webknjaz commented Jun 25, 2019

jaraco commented Sep 2, 2019

jaraco left a comment

Choose a reason for hiding this comment

webknjaz Sep 23, 2019

Choose a reason for hiding this comment

the-allanc Sep 30, 2019

Choose a reason for hiding this comment

webknjaz commented Sep 25, 2019

the-allanc commented May 22, 2019 •

edited by webknjaz

the-allanc commented May 22, 2019 •

edited by webknjaz

codecov bot commented Jun 25, 2019 •

edited

the-allanc commented Jun 25, 2019 •

edited by webknjaz