Ticket #1741 defect closed fixed

Opened 8 years ago

Last modified 8 years ago

threadCallQueue loop can grow list forever

Reported by: ghazel Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: spiv, exarkun, glyph, jknight Branch:
Author: Launchpad Bug:

Description

callFromThread adds an event to self.threadCallQueue, then runUntilCurrent loops over that list calling events until the list is empty. If one of those events uses callFromThread, the list grows forever.

Now, I know there's no need to use callFromThread in this case since it's occurring in the main thread, but a very simple modification makes it work anyway.

foom preferred the method I implemented: (alus): I think the loop should be written to iterate over a slice (alus): or take a copy, or stop after a number (alus): or something to prevent this (foom): yes, it should get the count when it starts. (foom): and then take that many and stop (foom): and then delete those (alus): ok, that modification fixes the leak

Also included in the patch is a method to re-call wakeUp when more events were added during iteration.

Attachments

callFromThread.diff Download (0.9 KB) - added by ghazel 8 years ago.
patch for callFromThread problem
callFromThread2.diff Download (2.4 KB) - added by jknight 8 years ago.
patch with test

Change History

Changed 8 years ago by ghazel

patch for callFromThread problem

1

Changed 8 years ago by ghazel

  • summary changed from callFromThread consumes memory to threadCallQueue loop can grow list forever

2

Changed 8 years ago by spiv

  • cc spiv added

Before this patch can be reviewed and committed, it needs to include a test case.

3

Changed 8 years ago by glyph

Going through my tickets list again tonight, I realized that I would prefer a different behavior than this ticket suggests.

callFromThread's docstring says that you should call it from non-reactor threads; I think that should actually be enforced. After all, if you are doing things in the wrong thread, I bet that is actually a serious error.

So, I won't reject this, since I think callFromThread should actually be modified to prevent this bug from happening, by raising an exception if you call it in the IO thread. But I don't think that this is a valid use-case.

4

Changed 8 years ago by glyph

  • owner changed from glyph to ghazel

5

Changed 8 years ago by exarkun

  • cc exarkun, glyph added

One use-case where this may actually be valid is within signal handlers. The reactors themselves use callFromThread in all of their own signal handlers. They can't easily know if this is precisely correct or not, since they don't know if the reactor is running in the thread to which signals will be delivered (always the "main" thread in CPython). They also don't know if they are being invoked halfway through some other statement: they need a completely re-entrant safe way to post an event to the reactor.

Changed 8 years ago by jknight

patch with test

6

Changed 8 years ago by jknight

  • keywords review added
  • cc jknight added

I disagree with glyph's comment. I think callFromThread should be the one API that is guaranteed to be *always* safe to call, no matter where you are. As JP points out, that is necessary for the reactor's internal use by signal handlers, and I think it should also be a guarantee provided to users.

I minorly modified ghazel's patch, and added a test.

7

Changed 8 years ago by jknight

  • priority changed from normal to highest
  • owner ghazel deleted

Awaiting review

8

Changed 8 years ago by exarkun

  • keywords review removed
  • owner set to jknight
  • The new test should be skipped if the reactor doesn't provide IReactorThreads
  • The comment at the top of the method should be a docstring
  • Change the whitespace to agree with the rest of the Twisted codebase (ie, spaces around assignment)

The actual code changes look good. Apply when the above are fixed.

9

Changed 8 years ago by ghazel

  • status changed from new to closed
  • resolution set to fixed

10

Changed 3 years ago by <automation>

  • owner jknight deleted
Note: See TracTickets for help on using tickets.