Ticket #3455 defect closed fixed

Opened 6 years ago

Last modified 5 years ago

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

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):

2

Changed 6 years ago by thijs

  • cc thijs added

3

Changed 5 years ago by exarkun

  • status changed from new to assigned
  • owner changed from glyph to exarkun

4

Changed 5 years ago by exarkun

  • branch set to branches/writesomedata-docs-3455
  • branch_author set to exarkun

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

5

Changed 5 years ago by exarkun

  • keywords documentation, review added; documentation removed
  • status changed from assigned to new
  • owner exarkun deleted

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.

6

Changed 5 years ago by mwh

  • owner set to exarkun
  • keywords documentation added; documentation, review removed

Looks fine to me. Please land.

7

Changed 5 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(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.

8

Changed 3 years ago by <automation>

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