Opened 9 years ago

Last modified 9 years ago

#5169 defect new

Races in _ThreadedWin32EventsMixin

Reported by: zseil Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

The recently added _ThreadedWin32EventsMixin has a couple of threading related bugs:

  • The callback added with addEvent could get called after the event has already been removed from the reactor.
  • The helper reactor could get completely blocked if one of the added events gets signaled while the main reactor is shutting down.
  • The helper reactor could get started before the main reactor is started.

I'll attach a series of patches that should fix these, they can also be pulled as patch branches from bitbucket: https://bitbucket.org/zseil/twisted-win32eventreactor

Please let me know if I should open a separate ticket for each of the patches or if they can go in together. I kept them separate mostly to ease the review.

Attachments (9)

01-test-cleanup.patch (4.2 KB) - added by zseil 9 years ago.
02-add-remove-race.patch (5.1 KB) - added by zseil 9 years ago.
03-fix-shutdown-races.patch (5.5 KB) - added by zseil 9 years ago.
04-delay-helper-start.patch (2.9 KB) - added by zseil 9 years ago.
05-further-test-cleanup.patch (4.4 KB) - added by zseil 9 years ago.
06-test-manual-reset-event.patch (1.2 KB) - added by zseil 9 years ago.
combined.patch (17.1 KB) - added by zseil 9 years ago.
03-fix-shutdown-races.2.patch (5.8 KB) - added by zseil 9 years ago.
updated patch
04-delay-helper-start.2.patch (2.9 KB) - added by zseil 9 years ago.
rereshed patch

Download all attachments as: .zip

Change History (13)

Changed 9 years ago by zseil

Attachment: 01-test-cleanup.patch added

Changed 9 years ago by zseil

Attachment: 02-add-remove-race.patch added

Changed 9 years ago by zseil

Attachment: 03-fix-shutdown-races.patch added

Changed 9 years ago by zseil

Attachment: 04-delay-helper-start.patch added

Changed 9 years ago by zseil

Changed 9 years ago by zseil

Changed 9 years ago by zseil

Attachment: combined.patch added

comment:1 Changed 9 years ago by zseil

Keywords: review added

Changed 9 years ago by zseil

updated patch

Changed 9 years ago by zseil

rereshed patch

comment:2 Changed 9 years ago by zseil

I've updated patch 3 to spin the reactor while waiting for the helper reactor thread instead of blocking in _reactorThread.join() and refreshed patch 4 to apply on top of it without any fuzz.

About my request for review, I would mostly like to know what the workflow is for a series of patches. Should I open one ticket per patch? Can the cleanup patches (patch 1, 5 and 6 in this series) be folded with functionality changing patches in that case? How should the dependency on earlier patches in the series be described in the ticket if there should be one ticket per patch?

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

Most people don't try to use a workflow based on patch series. So, you get to make it up. Try to pick something easy. :) If you figure something out that's worth sharing, feel free to write it up so others can benefit.

comment:4 Changed 9 years ago by zseil

Keywords: review removed

OK, I'll try with multiple tickets with cleanup patches folded into bugfix patches and put them to review one by one, in series order. Hopefully that won't be too painful. I'll close this ticket once I've created the replacement tickets.

Note: See TracTickets for help on using tickets.