Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6521 defect closed fixed (fixed)

StringTransportWithDisconnection.connectionLost should use a failure

Reported by: Adi Roiban Owned by: therve
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/string-transport-disconnection-failure-6521
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

The current implementation of t.test.proto_helpers.StringTransportWithDisconnection.connectionLost is calling self.protocol.connectionLost() with an Exception and not a Failure.

Change History (8)

comment:1 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I am trying to fix this issue.

To be able to commit my changes I have published them on GitHub.

The diff can be downloaded from here: https://github.com/chevah/twisted/compare/6521-StringTransportWithDisconnection-use-failure.diff

I have also created a Pull Request on GitHub in case it is of any help https://github.com/twisted/twisted/pull/1

I have fixed the implementation and also updated the failing tests.

There is still one un-caught exceptions but I don't know how to track it

$ trial twisted.test

....

[ERROR]
Traceback (most recent call last):
Failure: twisted.internet.error.ConnectionDone: Connection was closed cleanly: Bye..

twisted.test.test_pb.BrokerTestCase.test_cache
-------------------------------------------------------------------------------
Ran 2074 tests in 30.939s

FAILED (skips=58, expectedFailures=4, errors=1, successes=2011)



$ trial twisted.test.test_pb

...

    test_sync ...                                                          [OK]

-------------------------------------------------------------------------------
Ran 45 tests in 0.527s

PASSED (successes=45)

Any suggestion about how to look for fixing that exception is much appreciated.

Thanks!

comment:2 Changed 5 years ago by therve

Owner: set to therve

comment:3 Changed 5 years ago by therve

Author: therve
Branch: branches/string-transport-disconnection-failure-6521

(In [38508]) Branching to 'string-transport-disconnection-failure-6521'

comment:4 Changed 5 years ago by therve

Keywords: review removed

Looks good, applying.

comment:5 Changed 5 years ago by therve

Resolution: fixed
Status: newclosed

(In [38517]) Merge string-transport-disconnection-failure-6521

Author: adiroiban Reviewer: therve Fixes: #6521

Pass a Failure instead of a plain exception in StringTransportWithDisconnection.loseConnection.

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

Might have been nice to mention the apparently random other changes in the commit message. I *almost* reverted this change, because it looked like you just had some extra junk lying around in your trunk working copy that got committed by accident.

comment:7 Changed 5 years ago by Adi Roiban

Uh... I forgot about the news file.

Was the flaky test fixed? If yes, how? or that strange behavior only occur on my system.

Thanks!

comment:8 Changed 5 years ago by therve

Exarkun: sorry, I got lost in cleanup maze.

Adiroiban: yeah there was another missing assertFailure. You could track it relatively successfully using trial --force-gc.

Note: See TracTickets for help on using tickets.