Opened 11 years ago

Closed 11 years ago

#3455 defect closed fixed (fixed)

CONNECTION_LOST not an Integer...

Reported by: mforbes Owned by:
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Thijs Triemstra Branch: branches/writesomedata-docs-3455
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun


Several docstrings, notably those for the writeSomeData() method of t.i.a.FileDescriptor, claim that the return value is an integer reporting the number of bytes sent, or a negative integer reporting that the connection was lost.

The implementations, however, return t.i.main.CONNECTION_LOST which is an exception, not an integer.

Apart from the incorrect docstrings, returning an exception makes it quite difficult to test for errors. For example:

bytes_sent = self.connection.writeSomeData(...)
if 0 < bytes_sent:
    Still get here, even if connection is lost.

One has to check if the return value is explicitly CONNECTION_LOST, or if it is an instance of int etc.

Suggestion: Have writeSomeData throw the exception, or revert back to using an integer...

A quick grep indicates that this would require changes in the following files.

./Twisted/twisted/internet/ ./Twisted/twisted/internet/ ./Twisted/twisted/internet/ ./Twisted/twisted/internet/ ./Twisted/twisted/internet/ ./Twisted/twisted/internet/ ./Twisted/twisted/protocols/ ./Twisted/twisted/protocols/ ./Twisted/twisted/test/ ./Twisted/twisted/test/

Change History (8)

comment:1 Changed 11 years ago by therve

Keywords: documentation added; CONNECTION_LOST writeSomeData removed

Sorry, this is a documentation issue, but the behavior won't change, for several reasons (compatibility, performance, and others). Note that the fix is quite simple:

bytes_sent = self.connection.writeSomeData(...)
if bytes_sent < 0 or isinstance(bytes_sent, Exception):

comment:2 Changed 11 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:3 Changed 11 years ago by Jean-Paul Calderone

Owner: changed from Glyph to Jean-Paul Calderone
Status: newassigned

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

Author: exarkun
Branch: branches/writesomedata-docs-3455

(In [25420]) Branching to 'writesomedata-docs-3455'

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

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

Turns out, no implementation of writeSomeData returns a negative integer to indicate connection loss.

I could include this hunk in the branch if I wanted:

  • twisted/internet/

    111111            l = self.writeSomeData(buffer(self.dataBuffer, self.offset))
    112112        else:
    113113            l = self.writeSomeData(self.dataBuffer)
    114         if l < 0 or isinstance(l, Exception):
     114        if isinstance(l, Exception):
    115115            return l
    116116        if l == 0 and self.dataBuffer:
    117117            result = 0

without breaking any of Twisted's unit tests, but I'm not going to because perhaps some third-party writeSomeData implementation decided to take the negative return value documentation to heart and do that instead of returning an exception.

So I've just corrected the documentation to say that writeSomeData may returned an integer or an exception and dropped all the text about negative integers.

comment:6 Changed 11 years ago by Michael Hudson-Doyle

Keywords: review removed
Owner: set to Jean-Paul Calderone

Looks fine to me. Please land.

comment:7 Changed 11 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [25452]) Merge writesomedata-docs-3455

Author: exarkun Reviewer: mwhudson Fixes: #3455

Change the API documentation for writeSomeData so that it does not claim negative values are returned to indicate connection lose. Instead document that an exception instance will be returned to indicate that.

comment:8 Changed 8 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.