Opened 15 years ago

Closed 15 years ago

#1985 defect closed fixed (fixed)

AMP command raises BadLocalReturn despite setting requiresAnswer = False

Reported by: Richard Wall Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Richard Wall Branch:
Author:

Description

I expected that if I set requiresAnswer = False on my amp.command, that I would not have to return any thing from the responder. Instead, BadLocalReturn is raised:

twisted.protocols.amp.BadLocalReturn: <bound method SimpleSymmetricCommandProtocol.cmdNoAnswerHello of <SimpleSymmetricCommandProtocol None at 0x-48c78d94>> returned None and <class 'twisted.test.test_amp.NoAnswerHello'> could not serialize it Traceback: exceptions.AttributeError: 'NoneType' object has no attribute 'items'

Attachments (2)

amp-requiresanswer.py (713 bytes) - added by Richard Wall 15 years ago.
A simple example of how I came across this.
amp_requiresanswer.diff (1.1 KB) - added by Richard Wall 15 years ago.
A patch to the amp tests. I f I understand correctly, I don't think the NoAnswerHello command ever gets used because it's responder method is never called.

Download all attachments as: .zip

Change History (15)

Changed 15 years ago by Richard Wall

Attachment: amp-requiresanswer.py added

A simple example of how I came across this.

Changed 15 years ago by Richard Wall

Attachment: amp_requiresanswer.diff added

A patch to the amp tests. I f I understand correctly, I don't think the NoAnswerHello command ever gets used because it's responder method is never called.

comment:1 Changed 15 years ago by Jean-Paul Calderone

Milestone: Twisted-2.5

comment:2 Changed 15 years ago by Glyph

Owner: changed from Glyph to radix

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

There are really three problems here:

  • Responders for Commands defined to not require a response should be required to return None, but they are not
  • Returning None when a dict is expected or vice versa should result in more well-defined behavior (not a `NoneType has no attribute items' or whatever other random implementation-specific exception happens to come up)
  • The tests are buggy, since they do things which would be disallowed by one of the previous points.

comment:4 Changed 15 years ago by Glyph

Owner: changed from radix to Glyph
Status: newassigned

Man chris is so lazy. I guess I will have to do this.

comment:5 Changed 15 years ago by Glyph

The real issue here is that there is some misunderstanding of how requiresAnswer is supposed to work. There is some missing test coverage and documentation, which I will add - but the existing behavior is almost correct.

comment:6 Changed 15 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

The existing behavior was in fact exactly correct. Responding to exarkun's three points:

  • Responders for Commands defined not to require a response should return a valid response nonetheless, because requiresAnswer is an optimization hint that the client can specify, on any request whose response it will not process, to optimize network traffic.
  • Returning None when a dict is expected already logs a precise error, BadLocalReturn, and as the original reporter's bug says, the error message begins with "<your method> returned <your value> and <your command> could not serialize it". It then includes an implementation-specific traceback which could probably be elided for a value of None, but might be useful in the case of more obscure return values which are supposed to masquerade as dictionaries but are not functioning properly. (More information is better than less, and the more relevant information is already appearing at the front of the log message.)
  • The tests were not buggy - they are all testing valid behavior - but there was no test verifying the behavior of a requiresAnswer=False when an answer was requested nonetheless. I've added some tests, and I've added some notes in the documentation to clarify.

Ready for review in nofix-1985.

comment:7 Changed 15 years ago by Glyph

Milestone: Twisted-2.5

Somebody should still review this, but now that the issue has been analyzed more thoroughly, it is clear that it should not block the release.

comment:8 Changed 15 years ago by Stephen Thorne

Owner: set to Glyph

Please merge this forward, the docstring _AmpParserBase.callRemote has a conflict.

Thanks for updating the tests and the docstrings. I'm happy with signing off on this after the callRemote docstring is updated. Please merge forward, run the unit tests again, and merge.

comment:9 Changed 15 years ago by Glyph

Owner: changed from Glyph to Stephen Thorne

Done

comment:10 Changed 15 years ago by radix

Keywords: review removed
Owner: changed from Stephen Thorne to Glyph

Ok, apparently glyph didn't notice that Jerub said it was cool to merge this after fixing the docstring, probably because he left in the review keyword. I'll reassign this back to glyph and delete the review keyword to see what happens...

comment:11 Changed 15 years ago by Stephen Thorne

Looks good, yeah, merge nofix-1985-2 please.

comment:12 Changed 15 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [19086]) Enhance documentation and testing of requiresAnswer argument.

The requiresAnswer attribute of Command was previously documented as if to suggest that it had symmetric repercussions on the client and server, and the error-checking logic associated with it was not tested anywhere.

This change adds tests for detecting errors and improves the documentation somewhat.

Author: glyph

Reviewer: jerub

Fixes #1985

comment:13 Changed 11 years ago by <automation>

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