Opened 6 years ago

Closed 6 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
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

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 6 years ago.
util.py.2.patch (546 bytes) - added by zectbumo 6 years ago.
this also seems to fix it
test_pb.py.patch (1.2 KB) - added by zectbumo 6 years ago.

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by zectbumo

comment:1 Changed 6 years ago by therve

  • Component changed from core to pb
  • Keywords review added
  • Owner glyph deleted

Changed 6 years ago by zectbumo

this also seems to fix it

comment:2 Changed 6 years ago by zectbumo

I like patch 2 better since it is more straight forward

comment:3 follow-up: Changed 6 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to zectbumo
  • Priority changed from normal to highest

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 6 years ago by zectbumo

comment:4 in reply to: ↑ 3 Changed 6 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 6 years ago by zectbumo

  • Owner changed from zectbumo to therve

comment:6 Changed 6 years ago by therve

  • Keywords review removed
  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r22475.

comment:7 Changed 6 years ago by therve

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

Refs #3023

comment:8 Changed 6 years ago by therve

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:9 Changed 6 years ago by therve

  • author set to therve
  • Branch set to branches/filepager-empty-file-3023

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

comment:10 Changed 6 years ago by therve

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

Refs #3023

comment:11 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • Status changed from reopened to new

comment:12 Changed 6 years ago by exarkun

  • 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 6 years ago by therve

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

(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 3 years ago by <automation>

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