Opened 6 years ago

Closed 6 years ago

#3200 regression closed fixed (fixed)

Update iocp reactor to manage correctly the new _stopped flag

Reported by: therve Owned by:
Priority: high Milestone: Twisted-8.1
Component: core Keywords:
Cc: Branch: branches/iocp-fix-3200
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

Since r23345, ReactorBase now uses a new _stopped flag to correctly manage its running state. We should update the iocp reactor to use it, because it's not working correctly anymore.

The correct workflow might have been to revert the changeset, but we concluded that it could be bypassed here.

Change History (9)

comment:1 Changed 6 years ago by therve

  • author set to therve
  • Branch set to branches/iocp-fix-3200

(In [23378]) Branching to 'iocp-fix-3200'

comment:2 Changed 6 years ago by therve

  • Keywords review added
  • Owner changed from therve to exarkun

Please have a look.

comment:3 Changed 6 years ago by therve

  • Summary changed from Update iocp reactor the manage correctly the new _stopped flag to Update iocp reactor to manage correctly the new _stopped flag

comment:4 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to therve

This looks pretty good.

Can you add a bit to the _SignalReactorMixin docstring? The mixin expects some methods to be implemented on the class mixing it in - eg, sigInt, sometimes sigBreak, callLater, mainLoop. It also pretty much requires that it be mixed in with ReactorBase, since it calls up to a method of that class. Maybe it would make more sense if there to be _SignalReactorBase subclassing ReactorBase and subclassed by everyone else?

comment:5 Changed 6 years ago by therve

(In [23390]) Add some doc, remove some more duplication.

Refs #3200

comment:6 Changed 6 years ago by therve

  • Keywords review added
  • Owner changed from therve to exarkun

Okay, I updated documentation a bit. It's not awesome yet, so if you have some more idea don't hesitate. I didn't change the class hierarchy for now, because I think this is in flux so we just have to wait for something a little bit better. Please review.

comment:7 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to therve

Looks good to me, please merge.

comment:8 Changed 6 years ago by therve

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

(In [23392]) Merge iocp-fix-3200

Author: therve
Reviewer: exarkun
Fixes #3200

Fix the iocpreactor management of the new _stopped attribute introduced
in r23345. It removes a fair amount of duplication between iocpreactor and
posixbase in the process.

comment:9 Changed 3 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.