Opened 2 years ago

Closed 16 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 22 months ago.
5981_1.patch (4.0 KB) - added by ephess 22 months ago.
Patch addresses issues raised by review.
5981v3.patch (2.2 KB) - added by stephsolis 18 months ago.
Addressed more issues from review and added news file

Download all attachments as: .zip

Change History (21)

comment:1 Changed 22 months 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 22 months ago by ephess

comment:2 Changed 22 months ago by exarkun

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

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

comment:3 Changed 22 months ago by exarkun

(In [36613]) Apply 5981_0.patch

refs #5981

comment:4 Changed 22 months 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 22 months ago by ephess

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

Changed 22 months ago by ephess

Patch addresses issues raised by review.

comment:6 Changed 22 months 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 21 months ago by therve

(In [36753]) Apply new patch

Refs: #5981

comment:8 Changed 20 months ago by glyph

Hey have some build results

comment:9 Changed 20 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 20 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 18 months ago by stephsolis

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

Changed 18 months ago by stephsolis

Addressed more issues from review and added news file

comment:13 Changed 18 months ago by stephsolis

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

comment:14 Changed 17 months ago by therve

(In [38442]) Apply patch

Refs #5981

comment:15 Changed 17 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 16 months ago by glyph

Review, un-assigning.

comment:17 Changed 16 months ago by glyph

  • Owner therve deleted

comment:18 Changed 16 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.