Opened 16 years ago

Closed 15 years ago

#1843 defect closed fixed (fixed)

twisted.spread.util.FilePager keeps all sent chunks in memory

Reported by: ino Owned by:
Priority: highest Milestone:
Component: pb Keywords:
Cc: Branch:
Author:

Description

When sending a file, t.s.u.FilePager stores each chunk read (from the producer t.p.b.FileSender) in a list. This becomes a problem when the file being sent is very large, as it uses up a lot of memory.

A simple patch for this is attached. This patch pops from the chunks-list when sending, so that sent data isn't kept in memory.

Attachments (1)

pop-chunks-1843.diff (813 bytes) - added by ino 16 years ago.
pop chuncks when sending

Download all attachments as: .zip

Change History (9)

Changed 16 years ago by ino

Attachment: pop-chunks-1843.diff added

pop chuncks when sending

comment:1 Changed 16 years ago by Glyph

This bugfix needs an accompanying unit test.

comment:2 Changed 15 years ago by therve

Owner: changed from warner to therve

comment:3 Changed 15 years ago by therve

Keywords: review added
Owner: therve deleted
Priority: normalhighest

It's ready to review in pb-pager-1843+2321. I did a lot of cleanups because test_pb was a mess, so I hope it won't perturb review too much. I can split this if necessary, but it has to be done I think.

comment:4 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve
  • twisted.spread.util.FilePager.sendNextPage should have a docstring
  • twisted.test.test_pb.createFactoryCopy should have a docstring
  • NewStyleTestCase.setUp and .tearDown should have docstrings
  • If NewStyleTestCase.setUp called getRootObject, added a callback to save the root as an attribute of itself, and returned the Deferred, each of the test methods would be simplified somewhat.
  • NewStyleTestCase.test_newStyle and .test_alloc should have docstrings. Probably all the callback methods should as well, but you didn't modify them so you don't have to add them.
  • NewStyleCachedTestCase.test_newStyleCache should have a docstring
  • Now I know about twisted.spread.publish and I didn't before :(
  • The stuff moved out of test_spread should have docstrings. You didn't really change it, so you probably don't have to add them in this branch, but svn blame will say you wrote this undocumented code now, if you don't add them. ;)

The sendNextPage change and test look good.

comment:5 Changed 15 years ago by therve

Keywords: review added
Owner: changed from therve to Jean-Paul Calderone

I think I've addressed everything, and tried to document the most I could. Thanks!

comment:6 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve

Looks great, please merge

comment:7 Changed 15 years ago by therve

Resolution: fixed
Status: newclosed

(In [20609]) Merge pb-pager-1843+2321

Authors: ino, therve Reviewer: exarkun Fixes #1843 Fixes #2321

Modify FilePager so that data read is not kept in memory. In the process, do a lot of cleanups in test_pb to match current coding standards.

comment:8 Changed 11 years ago by <automation>

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