Opened 12 years ago

Closed 12 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, Jean-Paul Calderone, Glyph, jknight Branch:


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 12 years ago.
patch for callFromThread problem
callFromThread2.diff (2.4 KB) - added by jknight 12 years ago.
patch with test

Download all attachments as: .zip

Change History (12)

Changed 12 years ago by ghazel

Attachment: callFromThread.diff added

patch for callFromThread problem

comment:1 Changed 12 years ago by ghazel

Summary: callFromThread consumes memorythreadCallQueue loop can grow list forever

comment:2 Changed 12 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 12 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 12 years ago by Glyph

Owner: changed from Glyph to ghazel

comment:5 Changed 12 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone 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 12 years ago by jknight

Attachment: callFromThread2.diff added

patch with test

comment:6 Changed 12 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 12 years ago by jknight

Owner: ghazel deleted
Priority: normalhighest

Awaiting review

comment:8 Changed 12 years ago by Jean-Paul Calderone

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 12 years ago by ghazel

Resolution: fixed
Status: newclosed

comment:10 Changed 7 years ago by <automation>

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