Opened 2 years ago

Closed 18 months ago

#5981 enhancement closed fixed (fixed)

Clean up twisted.python.threadpool implementation using context managers

Reported by: itamar Owned by:
Priority: low Milestone:
Component: core Keywords: review
Cc: fs@… Branch: branches/threadpool-contextmanager-5981-2
(diff, github, buildbot, log)
Author: exarkun, glyph Launchpad Bug:

Description

Some parts of threadpool.py would be clearer with context managers. For example, in _worker, with working(self): ... instead of matched append/remove calls.

Attachments (3)

5981_0.patch (4.2 KB) - added by ephess 2 years ago.
5981_1.patch (4.0 KB) - added by ephess 2 years ago.
Patch addresses issues raised by review.
5981v3.patch (2.2 KB) - added by stephsolis 20 months ago.
Addressed more issues from review and added news file

Download all attachments as: .zip

Change History (21)

comment:1 Changed 2 years ago by ephess

  • Cc fs@… added
  • Keywords review added

Patch attached for sections of the _worker method. Have reviewed the rest of this module and can't find any other areas which look like they'd be more readable with a context manager - let me know if I missed anything.

Changed 2 years ago by ephess

comment:2 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/threadpool-contextmanager-5981

(In [36612]) Branching to 'threadpool-contextmanager-5981'

comment:3 Changed 2 years ago by exarkun

(In [36613]) Apply 5981_0.patch

refs #5981

comment:4 Changed 2 years ago by exarkun

  • Keywords review removed

Thanks! This looks like a great patch. I have just a few minor comments:

  1. Some names added in the patch don't follow the naming convention - we use camelCase rather than under_scores for variable names (amongst other things).
  2. A direct unit test or two for _workerState would be great, particularly for the exception case, which I don't think is currently exercised by the unit tests.
  3. This version involves a few extra calls to currentThread. That probably doesn't matter, but since it's not too hard to avoid the calls (ie, make it an argument to _workerState), it might be nice.

I checked the patch into a branch. If you can make the next patch against that branch rather than against trunk, that'd be great. Thanks again!

(cannot reassign properly, ephess is not in the user list for some reason :/)

comment:5 Changed 2 years ago by ephess

  • Owner set to ephess
  • Status changed from new to assigned

Changed 2 years ago by ephess

Patch addresses issues raised by review.

comment:6 Changed 2 years ago by ephess

  • Keywords review added
  • Owner ephess deleted
  • Status changed from assigned to new

Thanks for the feedback.

  1. Fixed, hard habit to break!
  2. Added, nice catch - there was a bug there.
  3. Refactored to pass thread ID into context manager.

comment:7 Changed 23 months ago by therve

(In [36753]) Apply new patch

Refs: #5981

comment:8 Changed 22 months ago by glyph

Hey have some build results

comment:9 Changed 22 months ago by glyph

  • Author changed from exarkun to exarkun, glyph
  • Branch changed from branches/threadpool-contextmanager-5981 to branches/threadpool-contextmanager-5981-2

(In [36959]) Branching to 'threadpool-contextmanager-5981-2'

comment:11 Changed 22 months ago by tom.prince

  • Keywords review removed
  • Owner set to ephess

This looks good. A couple of minor comments:

  • test_workerState should probably be split into two tests, one for the non-exception case, and one for the exception case.
  • Also, it would be a good thing to check that _workerState doesn't change the exception that got raised (1/0 and ZeroDivisionError are often used in twisted test-suite for this).

comment:12 Changed 20 months ago by stephsolis

  • Owner changed from ephess to stephsolis
  • Status changed from new to assigned

Changed 20 months ago by stephsolis

Addressed more issues from review and added news file

comment:13 Changed 20 months ago by stephsolis

  • Keywords review added
  • Owner stephsolis deleted
  • Status changed from assigned to new

comment:14 Changed 19 months ago by therve

(In [38442]) Apply patch

Refs #5981

comment:15 Changed 19 months ago by therve

  • Keywords easy removed
  • Owner set to therve

Build results: buildbot.twistedmatrix.com/boxes-supported?branch=/branches/threadpool-contextmanager-5981-2

comment:16 Changed 18 months ago by glyph

Review, un-assigning.

comment:17 Changed 18 months ago by glyph

  • Owner therve deleted

comment:18 Changed 18 months ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [38584]) Merge threadpool-contextmanager-5981-2

Authors: stephsolis, ephess
Reviewers: tom.prince, exarkun, therve
Fixes: #5981

Cleanup threadpool implementation by using context managers.

Note: See TracTickets for help on using tickets.