Ticket #3922 (closed defect: fixed)

Opened 14 months ago

Last modified 9 months ago

twisted.protocols.amp.BinaryBoxProtocol.connectionLost chokes on reason = main.CONNECTION_DONE

Reported by: dotz Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: win32
Cc: exarkun Branch: branches/stdio-connection-lost-reason-3922
Author: exarkun Launchpad Bug:

Description

I am running ActivePython 2.6 on Windows Vista with Twisted Trunk 19/07/2009 and ampoule trunk 19/07/2009 with patched  https://bugs.launchpad.net/ampoule/+bug/401460

Trying to run ampoule-trunk/examples/pid.py example file shows, that amp.BinaryBoxProtocol chokes when reason is main.CONNECTION_DONE -- amp.py line 1992.

Why does it use reason.check(...) ? Where is that function documented, is it specified by any interface? As far as I know, 'reason' passed to connectionLost may be an ordinary Exception class, so I think there should be a check for that... 

2009-07-19 21:01:28+0200 [-] FROM 0:    Traceback (most recent call last):
2009-07-19 21:01:28+0200 [-] FROM 0:      File "<string>", line 20, in <module>
2009-07-19 21:01:28+0200 [-] FROM 0:
2009-07-19 21:01:28+0200 [-] FROM 0:      File "<string>", line 19, in main
2009-07-19 21:01:28+0200 [-] FROM 0:
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\base.py", line 1166, in run
2009-07-19 21:01:28+0200 [-] FROM 0:        self.mainLoop()
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\base.py", line 1175, in mainLoop
2009-07-19 21:01:28+0200 [-] FROM 0:        self.runUntilCurrent()
2009-07-19 21:01:28+0200 [-] FROM 0:    --- <exception caught here> ---
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\base.py", line 779, in runUntilCurrent
2009-07-19 21:01:28+0200 [-] FROM 0:        call.func(*call.args, **call.kw)
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\_pollingfile.py", line 72, in _pollEvent
2009-07-19 21:01:28+0200 [-] FROM 0:        workUnits += resource.checkWork()
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\_pollingfile.py", line 129, in checkWork
2009-07-19 21:01:28+0200 [-] FROM 0:        self.cleanup()
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\_pollingfile.py", line 134, in cleanup
2009-07-19 21:01:28+0200 [-] FROM 0:        self.lostCallback()
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\_win32stdio.py", line 59, in readConnectionLost
2009-07-19 21:01:28+0200 [-] FROM 0:        self.checkConnLost()
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\internet\_win32stdio.py", line 73, in checkConnLost
2009-07-19 21:01:28+0200 [-] FROM 0:        self.proto.connectionLost(main.CONNECTION_DONE)
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\ampoule-0.1.1-py2.6.egg\ampoule\child.py", line 12, in connectionLost
2009-07-19 21:01:28+0200 [-] FROM 0:        amp.AMP.connectionLost(self, reason)
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\protocols\amp.py", line 2229, in connectionLost
2009-07-19 21:01:28+0200 [-] FROM 0:        BinaryBoxProtocol.connectionLost(self, reason)
2009-07-19 21:01:28+0200 [-] FROM 0:      File "c:\python26\lib\site-packages\twisted-8.2.0-py2.6-win32.egg\twisted\protocols\amp.py", line 1992, in connectionLost
2009-07-19 21:01:28+0200 [-] FROM 0:        elif reason.check(ConnectionClosed) and self._justStartedTLS:
2009-07-19 21:01:28+0200 [-] FROM 0:    exceptions.AttributeError: 'ConnectionDone' object has no attribute 'check'

Attachments

fix-binaryboxprotocol.patch Download (0.8 KB) - added by dotz 14 months ago.
patch for #3922
patch-test-amp-binaryboxprotocol.2.patch Download (1.5 KB) - added by dotz 14 months ago.
test included
fix-3922.patch Download (0.6 KB) - added by dotz 10 months ago.
New and hopefully proper fix for this issue attached.

Change History

Changed 14 months ago by dotz

patch for #3922

Changed 14 months ago by dotz

test included

  Changed 14 months ago by therve

The AMP code is correct, the bug is in _win32stdio: it should pass a failure to connectionLost, not a exception (ie, failure.Failreu(CONNECTION_DONE), like it's done everywhere else.

  Changed 14 months ago by exarkun

  • keywords win32 added
  • milestone regular-releases deleted

  Changed 14 months ago by dotz

How about we just fix it in the trunk? I don't think we can add tests, that test API-misuse... correct me if I'm wrong (with some details, perhaps) and I'll see if I can come up with a patch.

in reply to: ↑ description   Changed 13 months ago by dotz

Do you need anything else from me to get this one small change merged?

  Changed 12 months ago by dotz

Do you need anything else from me to get this one small change merged?

  Changed 12 months ago by glyph

  • owner changed from glyph to dotz

The point of tests for API misuse is to test that, when presented with bad input, a library will give helpful debugging error messages and exceptions to the developer who is attempting to use it. It's just the same as adding tests for good error messages when a user clicks the wrong button, or hits submit without fully filling out a form.

So yes, we can, and do, have plenty of tests for API misuse :).

This ticket doesn't appear to be in review; if you want to get stuff merged, please follow the development process outlined in TwistedDevelopment - i.e. add the 'review' keyword, reassign to nobody (the empty entry in the "assign to" field).

Thanks for all your contributions, sorry that the process has been getting in the way here. Hopefully we can get all these minor fixes integrated soon.

Changed 10 months ago by dotz

New and hopefully proper fix for this issue attached.

  Changed 10 months ago by dotz

New and hopefully proper fix for this issue attached.

I don't have a slightest idea how to create a test for this issue. Really. If we want to do as Glyph said and make the library developer-friendly, we would need a type check for value passed to connectionLost.

If anybody is able to help me create a test for this thing, please do.

  Changed 10 months ago by dotz

  • owner dotz deleted
  • keywords review added

  Changed 10 months ago by exarkun

  • owner set to dotz
  • keywords review removed
  • cc exarkun added

It seems likely that at least one of the tests in twisted.test.test_stdio must cover this already - but is probably missing an assertion about it or is being tricked by the automatic-Failure-wrapping-and-errbacking behavior of Deferred.callback. I'd take a look at the tests there and find one that has its connection lost reason in an easy-to-make-assertions-about place, and then add the assertion there.

Thanks!

  Changed 10 months ago by exarkun

(In [27581]) Change the stdio loseConnection test to also check the failure passed to connectionLost

refs #3922

  Changed 10 months ago by exarkun

(In [27583]) Apply dotz's proposed fix

refs #3922

  Changed 10 months ago by exarkun

  • keywords review added
  • owner dotz deleted

Okay, I expanded one of the tests to verify this behavior. I  built on Windows with just the test changes, and then I  rebuilt with the fix.

  Changed 10 months ago by exarkun

  • branch set to branches/stdio-connection-lost-reason-3922
  • branch_author set to exarkun

(In [27580]) Branching to 'stdio-connection-lost-reason-3922'

  Changed 9 months ago by therve

  • keywords review removed
  • owner set to exarkun

Please merge.

  Changed 9 months ago by exarkun

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

(In [27594]) Merge stdio-connection-lost-reason-3922

Author: dotz, exarkun Reviewer: therve Fixes: #3922

Change Windows stdio support so that it passes a Failure to the protocol's connectionLost method, rather than an exception. Also adjust one of the existing stdio tests to verify this behavior.

Note: See TracTickets for help on using tickets.