Opened 10 years ago

Closed 6 years ago

#2491 task closed fixed (fixed)

Deprecate special case behavior for non-None IProtocol.dataReceived return value handling

Reported by: Jean-Paul Calderone Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: Tom Davis Branch: branches/no-datareceived-result-2491
branch-diff, diff-cov, branch-cov, buildbot
Author: michaelnt

Description

There is no documentation for what IProtocol.dataReceived may return. Most of the time, None is the only valid return value. However, some transports handle other return values to mean other special things (like a reason for disconnection). We should just get rid of this behavior, after a reasonable period of deprecation.

Attachments (5)

2491.patch (3.4 KB) - added by Michael 7 years ago.
2491-1.patch (4.0 KB) - added by Michael 7 years ago.
deprecateFunction.patch (7.3 KB) - added by Michael 6 years ago.
registry.patch (8.9 KB) - added by Michael 6 years ago.
no-module-globals.patch (6.4 KB) - added by Michael 6 years ago.

Download all attachments as: .zip

Change History (48)

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

Keywords: easy added
Priority: normalhigh

The ticket summary says "deprecate and remove" but let's just say this ticket is only for the "deprecate" phase.

This would be nice to take care of. It's one of the ugly corners of IProtocol that will hamper adoption of the interface by other people (something I would like to start pushing).

comment:2 Changed 8 years ago by Drew Smathers

Owner: changed from Glyph to Drew Smathers
Status: newassigned

comment:3 Changed 7 years ago by Tom Davis

Cc: Tom Davis added

I'd like to take this over since it sounds both easy and important, but there are a lot of places that dataReceived() is called. Generally, I can only find one place in twisted.internet that it is actually returned: t.i.tcp#460 in Connection.doRead(). Is this ticket asking that the return value be checked and a deprecation warning given in the case that it is not None? Or am I completely off the mark?

comment:4 Changed 7 years ago by Jean-Paul Calderone

Is this ticket asking that the return value be checked and a deprecation warning given in the case that it is not None?

Yes, just that.

comment:5 Changed 7 years ago by Glyph

Owner: changed from Drew Smathers to Tom Davis
Status: assignednew

Looks like you're the winner, then.

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

Summary: Deprecate and remove special case behavior for non-None IProtocol.dataRecived return value handlingDeprecate special case behavior for non-None IProtocol.dataReceived return value handling

comment:7 Changed 7 years ago by Tom Davis

This ticket is currently held up by me not knowing where to start whitebox testing for the tcp module. Literally, there is the matter of where the module should go; exarkun has recommended t.i.tcp.tests.test_tcp_connection. Further, I am not sure how one would test t.i.tcp.Connection.doRead as I have insufficient experience testing low-level, internal APIs.

Exarkun's feedback in IRC:

  • Probably just instantiate Connection()
  • Use a fake socket; there is no current implementation and this should be "verified" (?)

Any guidance or a place to look for similar implementations would be appreciated. I'm familiar with emitting / checking for warnings, just not the specifics of test scaffolding for low-level classes.

Thanks!

comment:8 Changed 7 years ago by Michael

Keywords: review added
Owner: Tom Davis deleted

Added a patch which issues a DeprecationWarning if self.protocol.dataReceived returns something other than None and tests of this.

Not sure if the version string in the warning is the right version to use.

comment:9 in reply to:  8 Changed 7 years ago by Thijs Triemstra

Replying to michaelnt:

Not sure if the version string in the warning is the right version to use.

That should be 11.0, also see [ReleaseProcess#Versionnumbers].

comment:10 Changed 7 years ago by Michael

Updated the Twisted version number in the patch to 11,0,0.

P.S. Ran the complete Twisted test suite and this warning does not get raised by any of the existing tests.

comment:11 Changed 7 years ago by Tom Davis

L#63, the assert still assumes a deprecation version of 10.3. I didn't notice anything else, but then I didn't know how to write the TestCase in the first place ;)

Changed 7 years ago by Michael

Attachment: 2491.patch added

comment:12 Changed 7 years ago by Michael

Oops update the patch so that the Twisted version in the test is also 11,0,0

comment:13 Changed 7 years ago by Glyph

Owner: set to Michael

Hello again michaelnt, thanks for your many contributions :).

  1. The stuff in twisted.internet.test.test_tcpconnection should mostly go in twisted.internet.test.test_tcp. However, there should be a division between "fake stuff just for one test" (like the IProtocol that returns a non-None value) and "generalized in-memory implementations of other interfaces (like the "fake" socket). The latter category of stuff should probably live in twisted.test.proto_helpers, which is an unfortunately named module, but it's where all of our other in-memory stand-ins for things like the reactor and transports go, so it's the natural place for such a socket.
  2. All the tests need docstrings, explaining what they're testing and why. For example, there are lots of other tests which test doRead, so test_doread should say which case exactly it's covering and what the expected behavior is. There should definitely never be any empty or blank docstrings within Twisted.

comment:14 Changed 7 years ago by Glyph

Keywords: review removed

Changed 7 years ago by Michael

Attachment: 2491-1.patch added

comment:15 Changed 7 years ago by Michael

Keywords: review added
Owner: Michael deleted
  1. OK I moved the tests to twisted.internet.test.test_tcp. Is it worth trying to use the FakeSocket outside of this test? Wouldn't it need to implement more of the socket API to be generally useful. If I implement a more complete mock would it be used anywhere?
  1. Added some doc strings and renamed the test method.

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

Author: exarkun
Branch: branches/no-datareceived-result-2491

(In [30850]) Branching to 'no-datareceived-result-2491'

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

(In [30851]) Apply 2491-1.patch

refs #2491

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

Author: exarkunmichaelnt

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

(In [30860]) Minor reformatting to stay within 80 columns

refs #2491

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

Keywords: review removed
Owner: set to Michael

Thanks michael.

I think we should have a module for FakeSocket and such things. I don't think it's a pressing issue to get it in the right place immediately. This FakeSocket doesn't look like it will do much that will be generally useful quite yet.

I reformatted some code to get it into <80 columns, and tweaked some docstrings. More importantly though, I think emitting the deprecation warning with warnings.warn is going to give confusing results:

/home/exarkun/Projects/Twisted/trunk/twisted/internet/selectreactor.py:146: DeprecationWarning: Returning a value other than None from __main__.P.dataReceived was deprecated in Twisted 11.0.0
  why = getattr(selectable, method)()

All this really has going for it is that __main__.P.dataReceived is in there. That's enough to get people to the right place, but the location leading up to that and the line following it are just lies.

If we use warn_explicit instead we can point the warning at the definition of __main__.P.dataReceived:

foo.py:7: DeprecationWarning: Returning a value other than None from __main__.P.dataReceived is deprecated since Twisted 11.0.0.
  def dataReceived(self, bytes):

This is a little trickier, since warn_explicit requires more arguments than warn, and some of them take some effort to get. To prove to myself that it was really possible to do this with warn_explicit I implemented it, committed in r30863. I didn't actually implement one critical piece though, the part that finds the right line number to pass to warn_explicit. It also probably needs more tests, because maybe the module and registry and module_globals arguments can also affect the behavior (like what gets suppressed, whether duplicate warnings are emitted, etc).

Unless anyone disagrees about this form of the warning being more desirable, I think we should add the necessary getfirstlineno function to make this work and write a few more tests for those other args being correct.

Thanks again for working on this michael!

Changed 6 years ago by Michael

Attachment: deprecateFunction.patch added

comment:21 Changed 6 years ago by Michael

Keywords: review added
Owner: Michael deleted

Factored exarkun's code into a new deprecate.deprecateFunction and added tests.

Modified the code to work with python 2.4, tested on python 2.4 and 2.6.

Perhaps this code should live somewhere else?

There is some duplication between this new code and the TestCase.flushWarnings.

comment:22 Changed 6 years ago by Steve Waterbury

Keywords: easy removed

Since this is a patch by a non-committer, it shouldn't show up in the easy category, because other non-committer reviewers can't commit it. Committers: feel free to put the "easy" keyword back after you review it.

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

This is quite related to #2215.

comment:24 Changed 6 years ago by Drew Smathers

Keywords: review removed
Owner: set to Michael

I would like to review this but I think the patch needs to be redone - doesn't apply cleanly against a clean checkout of trunk now:

patching file twisted/internet/tcp.py
Reversed (or previously applied) patch detected!  Assume -R? [n]

comment:25 Changed 6 years ago by washort

Owner: changed from Michael to washort
Status: newassigned

comment:26 Changed 6 years ago by Michael

The patch is against the branch not against trunk

comment:27 Changed 6 years ago by washort

Owner: changed from washort to Michael
Status: assignednew

Looks pretty good; there's a few things that should change, though:

  1. FakeSocket has a blocking attribute; real sockets don't, so testing for its value seems unproductive.
  2. deprecateFunction sounds like a name for a decorator for deprecation. Maybe a better name would be blame?
  3. deprecateFunction's docstring says it takes a callable, but it's only tested on functions (and probably only makes sense for functions).

comment:28 Changed 6 years ago by washort

Keywords: review added

don't mind me, i'm just gaming the highscore system

comment:29 Changed 6 years ago by washort

Keywords: review removed

comment:30 in reply to:  27 Changed 6 years ago by Michael

Keywords: review added
Owner: Michael deleted

Replying to washort:

Looks pretty good; there's a few things that should change, though:

  1. FakeSocket has a blocking attribute; real sockets don't, so testing for its value seems unproductive.

t.i.tcp.Connection requires a socket to have a setblocking method so it is required.

  1. deprecateFunction sounds like a name for a decorator for deprecation. Maybe a better name would be blame?

blame is pretty vague, and only DeprecationWarnings are issued. deprecateFunction seems to fit better with the existing deprecateAttribute, but feel free to change it whilst committing.

  1. deprecateFunction's docstring says it takes a callable, but it's only tested on functions (and probably only makes sense for functions).

I don't think that's worth a patch.

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

(In [31094]) Apply deprecateFunction.patch

refs #2491

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

(In [31098]) whitespace-cleanup

refs #2491

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

(In [31100]) Some whitespace changes, some docstring twiddling

refs #2491

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

(In [31101]) Comment out some critical parts of the implementation which are not covered by the unit tests

refs #2491

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

(In [31102]) Adjust the usage of this function to match the renaming

refs #2491

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Michael
Status: assignednew

Thanks michael!

I did some of the boring changes, whitespace and tweaked some naming and docstrings. I think the only thing that's really left to do is add testing for the last three parameters to warnings.warn_explicit. I checked in a revision that comments them out, and the current tests all still pass.

It's possible that the values I was passing aren't even really necessary, I'm not totally sure. I think __warningregistry__ provides the feature of suppressing the second and later instances of a particular warning. I'm not sure about __module__ and module_globals though. If we can't figure out what difference they make, I guess we shouldn't bother trying to pass any particular value for this parameters.

Changed 6 years ago by Michael

Attachment: registry.patch added

Changed 6 years ago by Michael

Attachment: no-module-globals.patch added

comment:38 Changed 6 years ago by Michael

Keywords: review added
Owner: Michael deleted

Please ignore the registry patch, unless you're really interested in trying to load lines of source code from modules loaded from zipfiles and displaying these lines in warning messages (which unittest doesn't support).

The no-module-globals.patch adds tests for the registry attribute and sets module_globals to None, which is what the C (builtin) implemetation of warnings.warn does.

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

(In [31185]) Apply no-module-globals.patch

refs #2491

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

Keywords: review removed

Thanks! I am sad about the behaviour of the C implementation of warnings.warn but I guess I am happy it means we don't have to do extra work to implement more silly features. ;)

There was one issue with sys.modules state being shared between some tests in twisted.trial and twisted.python and causes failures. I fixed it by resetting sys.modules completely at the end of the test and that seemed to help.

I'm going to test this on buildbot and if that goes well, merge. Thanks again!

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

Argh, I should have known, Windows filename issues. I'll see if they're easy to fix.

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

Across a number of revisions far greater than should have been necessary, the tests work on Windows now. It was just necessary to construct path strings using the native directory separator (I let FilePath do it) and then normcase before comparing. This was a test change only.

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

Resolution: fixed
Status: newclosed

(In [31210]) Merge no-datareceived-result-2491

Author: michaelnt, exarkun Reviewer: exarkun, washort Fixes: #2491

Deprecate the disconnection behavior of returning any non-None value from IProtocol.dataReceived.

Also add a warning helper function, warnAboutFunction, for emitting a warning "at" a function which is not currently on the call stack.

Note: See TracTickets for help on using tickets.