Opened 5 years ago

Closed 4 years ago

Last modified 8 months ago

#5981 enhancement closed fixed (fixed)

Clean up twisted.python.threadpool implementation using context managers

Reported by: Itamar Turner-Trauring Owned by:
Priority: low Milestone:
Component: core Keywords:
Cc: Jordan Hagan Branch: branches/threadpool-contextmanager-5981-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, glyph

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 Jordan Hagan 4 years ago.
5981_1.patch (4.0 KB) - added by Jordan Hagan 4 years ago.
Patch addresses issues raised by review.
5981v3.patch (2.2 KB) - added by Stephen Solis 4 years ago.
Addressed more issues from review and added news file

Download all attachments as: .zip

Change History (22)

comment:1 Changed 4 years ago by Jordan Hagan

Cc: Jordan Hagan 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 4 years ago by Jordan Hagan

Attachment: 5981_0.patch added

comment:2 Changed 4 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/threadpool-contextmanager-5981

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

comment:3 Changed 4 years ago by Jean-Paul Calderone

(In [36613]) Apply 5981_0.patch

refs #5981

comment:4 Changed 4 years ago by Jean-Paul Calderone

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 4 years ago by Jordan Hagan

Owner: set to Jordan Hagan
Status: newassigned

Changed 4 years ago by Jordan Hagan

Attachment: 5981_1.patch added

Patch addresses issues raised by review.

comment:6 Changed 4 years ago by Jordan Hagan

Keywords: review added
Owner: Jordan Hagan deleted
Status: assignednew

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 4 years ago by therve

(In [36753]) Apply new patch

Refs: #5981

comment:8 Changed 4 years ago by Glyph

Hey have some build results

comment:9 Changed 4 years ago by Glyph

Author: exarkunexarkun, glyph
Branch: branches/threadpool-contextmanager-5981branches/threadpool-contextmanager-5981-2

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

comment:11 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Jordan Hagan

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 4 years ago by Stephen Solis

Owner: changed from Jordan Hagan to Stephen Solis
Status: newassigned

Changed 4 years ago by Stephen Solis

Attachment: 5981v3.patch added

Addressed more issues from review and added news file

comment:13 Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted
Status: assignednew

comment:14 Changed 4 years ago by therve

(In [38442]) Apply patch

Refs #5981

comment:15 Changed 4 years 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 4 years ago by Glyph

Review, un-assigning.

comment:17 Changed 4 years ago by Glyph

Owner: therve deleted

comment:18 Changed 4 years ago by therve

Resolution: fixed
Status: newclosed

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

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

Cleanup threadpool implementation by using context managers.

comment:19 Changed 8 months ago by hawkowl

Keywords: review removed

[mass edit] Removing the review tag from closed tickets

Note: See TracTickets for help on using tickets.