Opened 8 years ago

Closed 8 years ago

#1741 defect closed fixed (fixed)

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 (2)

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

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by ghazel

patch for callFromThread problem

comment:1 Changed 8 years ago by ghazel

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

comment: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.

comment: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.

comment:4 Changed 8 years ago by glyph

  • Owner changed from glyph to ghazel

comment: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

comment:6 Changed 8 years ago by jknight

  • Cc jknight added
  • Keywords review 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.

comment:7 Changed 8 years ago by jknight

  • Owner ghazel deleted
  • Priority changed from normal to highest

Awaiting review

comment: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.

comment:9 Changed 8 years ago by ghazel

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

comment:10 Changed 4 years ago by <automation>

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