Opened 9 years ago

Closed 9 years ago

#3023 defect closed fixed (fixed)

[PATCH] twisted.spread.util.FilePager doesn't callback when sending an empty file

Reported by: zectbumo Owned by:
Priority: highest Milestone:
Component: pb Keywords:
Cc: therve Branch: branches/filepager-empty-file-3023
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

If you use FilePager to send an empty file then the completion callback never gets called.

Attachments (3)

util.py.patch (821 bytes) - added by zectbumo 9 years ago.
util.py.2.patch (546 bytes) - added by zectbumo 9 years ago.
this also seems to fix it
test_pb.py.patch (1.2 KB) - added by zectbumo 9 years ago.

Download all attachments as: .zip

Change History (17)

Changed 9 years ago by zectbumo

Attachment: util.py.patch added

comment:1 Changed 9 years ago by therve

Component: corepb
Keywords: review added
Owner: Glyph deleted

Changed 9 years ago by zectbumo

Attachment: util.py.2.patch added

this also seems to fix it

comment:2 Changed 9 years ago by zectbumo

I like patch 2 better since it is more straight forward

comment:3 Changed 9 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to zectbumo
Priority: normalhighest

Thanks for the patch. Some comments:

  • while you're at it, please move Pager.__init__ in the first line of the __init__ method
  • your empty file doesn't need to be created in the setUp method as it's only used in your test. Please move the creation in your test, make it a local variable, and makes it follow twisted naming standard (filenameEmpty instead of self.filename_empty)
  • Use self.fail(msg) instead of raise Exception(msg)

Thanks!

Changed 9 years ago by zectbumo

Attachment: test_pb.py.patch added

comment:4 in reply to:  3 Changed 9 years ago by zectbumo

Keywords: review added

Replying to therve:

  • while you're at it, please move Pager.__init__ in the first line of the __init__ method

I wish I could. The problem gets bigger if I try that. Pager.init calls broker.registerPageProducer which eventually calls sendNextPage which needs a reference to self.chunks and self.chunks won't get initialize until after Pager.init. The problem comes down to that Pager.init is doing more than just initializing, it's also registering which causes wheels to start turning before we are done initializing.

  • your empty file doesn't need to be created in the setUp method as it's only used in your test. Please move the creation in your test, make it a local variable, and makes it follow twisted naming standard (filenameEmpty instead of self.filename_empty)
  • Use self.fail(msg) instead of raise Exception(msg)

done

comment:5 Changed 9 years ago by zectbumo

Owner: changed from zectbumo to therve

comment:6 Changed 9 years ago by therve

Keywords: review removed
Resolution: fixed
Status: newclosed

Fixed in r22475.

comment:7 Changed 9 years ago by therve

(In [22482]) Revert r22475: stupid print statement.

Refs #3023

comment:8 Changed 9 years ago by therve

Resolution: fixed
Status: closedreopened

comment:9 Changed 9 years ago by therve

author: therve
Branch: branches/filepager-empty-file-3023

(In [22485]) Branching to 'filepager-empty-file-3023'

comment:10 Changed 9 years ago by therve

(In [22486]) Apply the patch, without the print.

Refs #3023

comment:11 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted
Status: reopenednew

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

Keywords: review removed
Owner: set to therve

Humm okay. I wish the PB code and tests were nicer. :(

But a bug is fixed, so this should go in. Thanks, please merge.

comment:13 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(In [22557]) Merge filepager-empty-file-3023

Authors: zectbumo, therve Reviewers: exarkun, therve Fixes #3023

Make twisted.spread.util.FilePager works with empty files: previously, it didn't callback correctly.

comment:14 Changed 6 years ago by <automation>

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