Opened 6 years ago

Closed 6 years ago

#7989 defect closed fixed (fixed)

Twisted 15.3.0 regression pickling a lambda function in python 2

Reported by: Daniel Graña Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/picklambda-7989
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

Since twisted 15.3.0 importing twisted.persisted.styles alters how functions are pickled. This started to happen after #7827 because @qualname@ attribute doesn't exists on lambda functions for python 2 (it does for python3).

Relevant lines of #7827 change are https://github.com/twisted/twisted/blob/a8d8a0c89dc530554c768739e83d7a852cb17f22/twisted/persisted/styles.py#L116-L117

import sys
import twisted

try:
    # try python2 path first
    import cPickle as pickle
except ImportError:
    # fallback for python3
    import pickle


print(sys.version)
print(twisted.version)


def pickleit(msg):
    try:
        pickle.dumps((lambda x: x), protocol=2)
    except pickle.PicklingError:
        print("PicklingError raised -- That's expected for lambda functions")
    except Exception:
        print("PicklingError not raised! -- BUG")
        raise

pickleit("pickling lambda func")
import twisted.persisted.styles
print('twisted.persisted.styles imported')
pickleit("pickling lambda func")

In Twisted 15.3.0 it fails like:

~$ mkvirtualenv tx1530
New python executable in /home/daniel/envs/tx1530/bin/python2
Also creating executable in /home/daniel/envs/tx1530/bin/python
Installing setuptools, pip, wheel...done.

tx1530 ~$ pip install Twisted==15.3.0
cache.
Collecting Twisted==15.3.0
Collecting zope.interface>=3.6.0 (from Twisted==15.3.0)
Requirement already satisfied (use --upgrade to upgrade): setuptools in ./envs/tx1530/lib/python2.7/site-packages (from zope.interface>=3.6.0->Twisted==15.3.0)
Installing collected packages: zope.interface, Twisted
Successfully installed Twisted-15.3.0 zope.interface-4.1.2

tx1530 ~$ python picklinglambdabug.py 
2.7.10 (default, May 26 2015, 04:16:29) 
[GCC 5.1.0]
[twisted, version 15.3.0]
PicklingError raised -- That's expected for lambda functions
twisted.persisted.styles imported
PicklingError not raised! -- BUG
Traceback (most recent call last):
  File "picklinglambdabug.py", line 28, in <module>
    pickleit("pickling lambda func")
  File "picklinglambdabug.py", line 18, in pickleit
    pickle.dumps((lambda x: x), protocol=2)
  File "/home/daniel/envs/tx1530/lib/python2.7/site-packages/twisted/persisted/styles.py", line 117, in _pickleFunction
    tuple([".".join([f.__module__, f.__qualname__])]))
AttributeError: 'function' object has no attribute '__qualname__'

In Twisted 15.2.1 it doesn't fail:

~$ mkvirtualenv tx1521
New python executable in /home/daniel/envs/tx1521/bin/python2
Also creating executable in /home/daniel/envs/tx1521/bin/python
Installing setuptools, pip, wheel...done.

tx1521 ~$ pip install Twisted==15.2.1
DEPRECATION: --download-cache has been deprecated and will be removed in the future. Pip now automatically uses and configures its cache.
Collecting Twisted==15.2.1
Collecting zope.interface>=3.6.0 (from Twisted==15.2.1)
Requirement already satisfied (use --upgrade to upgrade): setuptools in ./envs/tx1521/lib/python2.7/site-packages (from zope.interface>=3.6.0->Twisted==15.2.1)
Installing collected packages: zope.interface, Twisted
Successfully installed Twisted-15.2.1 zope.interface-4.1.2

tx1521 ~$ python picklinglambdabug.py 
2.7.10 (default, May 26 2015, 04:16:29) 
[GCC 5.1.0]
[twisted, version 15.2.1]
PicklingError raised -- That's expected for lambda functions
twisted.persisted.styles imported
PicklingError raised -- That's expected for lambda functions

Change History (19)

comment:1 Changed 6 years ago by Daniel Graña

Type: enhancementregression

comment:2 Changed 6 years ago by Daniel Graña

Note: I can't find a way to edit ticket description so commenting instead.

__qualname__ seems to be a python3 only function's attribute, but somehow the code doesn't fail for non-lambda functions.

comment:3 Changed 6 years ago by Glyph

Type: regressiondefect

We use the type "regression" mainly to flag things that haven't already been in a release, and would thus block the next release. So while this is a regression from 15.2 to 15.3, workflow-wise we will treat it as a "defect" since it's already out there in the wild. Unless you think it's severe enough to require a micro-version release (15.3.1), but it doesn't seem like the case to me.

I'd also like to take this opportunity to gently remind our users that discovering regressions like this is why the pre-release window exists - if this had been discovered during the 15.3 prerelease cycle it could have blocked the release. Please do not mistake this reminder for a lack of gratitude, however; we appreciate the bug report nevertheless, whenever it is made :).

comment:4 Changed 6 years ago by Glyph

I am really curious though:

  1. how did you notice this, and
  2. why do you care? :)

comment:5 Changed 6 years ago by Glyph

Author: glyph
Branch: branches/picklambda-7989

(In [45396]) Branching to picklambda-7989.

comment:6 Changed 6 years ago by Glyph

Keywords: review added

comment:7 Changed 6 years ago by kmike

This issue makes Scrapy tests fail, and it can cause problems for its users.

Scrapy allows to store requests queue on disk and uses pickle to serialize requests. Each request must have a callback; a common issue is that if a callback is lamdba then it can't be stored on disk, while working with in-memory queues. Scrapy catches pickling errors and puts a request to the in-memory queue if it can't be serialized; also, a warning is emitted which describes what to do.

In 15.3.0 this is broken for two reasons:

  1. A wrong exception is raised, so Scrapy is not catching it. It means requests can be dropped instead of being put into an in-memory queue.
  2. In Python 3.3 no exception is raised at all when pickling lambdas, so requests can be stored incorrectly. Scrapy doesn't work in Python 3 yet, so only the first issue is a practical one.

Daniel started to create ugly workarounds (https://github.com/scrapy/scrapy/pull/1413) which we'll also have to backport to the previous Scrapy release. It'd be great to have this fixed in a Twisted release instead.

comment:8 Changed 6 years ago by Daniel Graña

The problem is that side effects of importing twisted.persisted.styles are activated by importing twisted.web.server module, which is far more common.

Scrapy doesn't use twisted.persisted at all, but when it imports twisted.web.server to provide a web console the bug is triggered and the serialization of requests goes mad.

comment:9 Changed 6 years ago by Glyph

Summary: Tx 15.3.0 regression pickling a lambda function in python 2Twisted 15.3.0 regression pickling a lambda function in python 2

("tx" is the prefix for community projects; "twisted" is what we're talking about here)

comment:10 Changed 6 years ago by Daniel Graña

| ("tx" is the prefix for community projects; "twisted" is what we're talking about here)

Good to know, thanks.

comment:11 Changed 6 years ago by Daniel Graña

I'd also like to take this opportunity to gently remind our users that discovering regressions like this is why the pre-release window exists - if this had been discovered during the 15.3 prerelease cycle it could have blocked the release.

How do I know when Twisted enters pre-release window? I'd like to setup an automated run of Scrapy testsuite against Twisted pre-releases, is testing against trunk good enough?

Please do not mistake this reminder for a lack of gratitude, however; we appreciate the bug report nevertheless, whenever it is made :).

No worries, your words are fine for me.

comment:12 in reply to:  11 Changed 6 years ago by Glyph

Replying to dangra:

I'd also like to take this opportunity to gently remind our users that discovering regressions like this is why the pre-release window exists - if this had been discovered during the 15.3 prerelease cycle it could have blocked the release.

How do I know when Twisted enters pre-release window?

Glad you asked! I'm sorry we haven't communicated this sufficiently well.

Our release manager sends an announcement email like this to the twisted-python@twistedmatrix.com mailing list every time a pre-release is made.

I'd like to setup an automated run of Scrapy testsuite against Twisted pre-releases, is testing against trunk good enough?

If you notify us of regressions as soon as they happen against trunk, that is even better than testing against a pre-release; ideally we'd just fix bugs quickly as they happen. The pre-release window is to give specific notice (one week) for those who have to do extra work to test against a recent version.

Please do not mistake this reminder for a lack of gratitude, however; we appreciate the bug report nevertheless, whenever it is made :).

No worries, your words are fine for me.

Thank you!

comment:13 Changed 6 years ago by Glyph

Builds are all green now, fix is pretty straightforward: IMHO an easy review :).

comment:14 Changed 6 years ago by Daniel Graña

| Builds are all green now, fix is pretty straightforward: IMHO an easy review :).

Excuse my ignorance, but I can't find a link to the fix. Only see you branched at https://twistedmatrix.com/trac/changeset/45396 but can't find following commits.

Who is supposed to be the reviewer?

comment:16 in reply to:  14 Changed 6 years ago by Glyph

Replying to dangra:

| Builds are all green now, fix is pretty straightforward: IMHO an easy review :).

Excuse my ignorance, but I can't find a link to the fix.

Definitely excused :).

Only see you branched at https://twistedmatrix.com/trac/changeset/45396 but can't find following commits.

At the top of the ticket you will see links such as "diff, github, buildbot, log". "diff" is the raw text diff, "github" is the diff on github's source browser, "buildbot" is a link to the buildbot builds, and "log" is the log of the branch's history in trac's source browser. So you can view the diff in any way that makes sense to you.

Who is supposed to be the reviewer?

You, of course! :)

There is a review queue - https://twistedmatrix.com/trac/report/25 - which Twisted contributors use to discover which tickets are available for review. In this case, I have commit access to Twisted, so anyone (either a committer or not) may review my contributions, and it's my responsibility to ensure the review is adequate. (If a non-committer contributor submits a patch, only a contributor may review.)

You can read about how to do a review on https://twistedmatrix.com/trac/wiki/ReviewProcess (the specific rule about who can review is documented in this section of that page). If you're not comfortable doing the review, someone else from the core team will eventually get to it.

comment:17 Changed 6 years ago by Daniel Graña

Keywords: review removed
Owner: set to Glyph

+1 please merge.

The fix is quite simple indeed, code is readable, tools builds are all green, and new test cases coverage is perfect.

Just to be sure I rerun the script used to reproduce the bug; it fails for trunk and passes for bugfix branch as expected.

~$ cd ~/src/Twisted
± Twisted trunk(ad778c2) ~/src/Twisted$ mkvirtualenv twisted7989
New python executable in /home/daniel/envs/twisted7989/bin/python2
Not overwriting existing python script /home/daniel/envs/twisted7989/bin/python (you must use /home/daniel/envs/twisted7989/bin/python2)
Installing setuptools, pip, wheel...done.

twisted7989 ± Twisted trunk(ad778c2) ~/src/Twisted$ pip install -e . 
Obtaining file:///home/daniel/src/Twisted
Requirement already satisfied (use --upgrade to upgrade): zope.interface>=3.6.0 in /home/daniel/envs/twisted7989/lib/python2.7/site-packages (from Twisted==15.3.0)
Requirement already satisfied (use --upgrade to upgrade): setuptools in /home/daniel/envs/twisted7989/lib/python2.7/site-packages (from zope.interface>=3.6.0->Twisted==15.3.0)
Installing collected packages: Twisted
  Running setup.py develop for Twisted
Successfully installed Twisted-15.3.0

twisted7989 ± Twisted trunk(ad778c2) ~/src/Twisted$ python ~/picklinglambdabug.py 
2.7.10 (default, May 26 2015, 04:16:29) 
[GCC 5.1.0]
[twisted, version 15.3.0]
PicklingError raised -- That's expected for lambda functions
twisted.persisted.styles imported
PicklingError not raised! -- BUG
Traceback (most recent call last):
  File "/home/daniel/picklinglambdabug.py", line 28, in <module>
    pickleit("pickling lambda func")
  File "/home/daniel/picklinglambdabug.py", line 18, in pickleit
    pickle.dumps((lambda x: x), protocol=2)
  File "/home/daniel/src/Twisted/twisted/persisted/styles.py", line 117, in _pickleFunction
    tuple([".".join([f.__module__, f.__qualname__])]))
AttributeError: 'function' object has no attribute '__qualname__'

twisted7989 ± Twisted trunk(ad778c2) ~/src/Twisted$ git checkout picklambda-7989
Switched to branch 'picklambda-7989'
Your branch is up-to-date with 'origin/picklambda-7989'.

twisted7989 ± Twisted picklambda-7989(64cb102) ~/src/Twisted$ python ~/picklinglambdabug.py 
2.7.10 (default, May 26 2015, 04:16:29) 
[GCC 5.1.0]
[twisted, version 15.3.0]
PicklingError raised -- That's expected for lambda functions
twisted.persisted.styles imported
PicklingError raised -- That's expected for lambda functions
Last edited 6 years ago by Daniel Graña (previous) (diff)

comment:18 Changed 6 years ago by Glyph

You did forget one detail in your review, but it was a minor one. Thanks for having a look; I will land this now.

comment:19 Changed 6 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [45406]) Merge picklambda-7989: raise PicklingError for lambdas

Author: glyph

Reviewer: dangra

Fixes: #7989

Fix a bug introduced in 15.3.0; pickling a lambda function after importing twisted.persisted.styles once again raises PicklingError rather than AttributeError.

Note: See TracTickets for help on using tickets.