Opened 6 years ago

Closed 6 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 Branch: branches/writesomedata-docs-3455
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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/_javaserialport.py
./Twisted/twisted/internet/_posixserialport.py
./Twisted/twisted/internet/abstract.py
./Twisted/twisted/internet/fdesc.py
./Twisted/twisted/internet/process.py
./Twisted/twisted/internet/tcp.py
./Twisted/twisted/protocols/htb.py
./Twisted/twisted/protocols/pcp.py
./Twisted/twisted/test/test_internet.py
./Twisted/twisted/test/test_pcp.py

Change History (8)

comment:1 Changed 6 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 6 years ago by thijs

  • Cc thijs added

comment:3 Changed 6 years ago by exarkun

  • Owner changed from glyph to exarkun
  • Status changed from new to assigned

comment:4 Changed 6 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/writesomedata-docs-3455

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

comment:5 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

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/abstract.py

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

  • Keywords review removed
  • Owner set to exarkun

Looks fine to me. Please land.

comment:7 Changed 6 years ago by exarkun

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

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

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