Code

Opened 3 years ago

Closed 2 years ago

Last modified 22 months ago

#16563 closed Bug (fixed)

Error pickling request.user

Reported by: zero.fuxor@… Owned by: nobody
Component: contrib.auth Version: 1.3
Severity: Release blocker Keywords:
Cc: bronger, kmike84@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by jezdez)

trying to pickle a request.user in trunk raises: TypeError, can't pickle function objects
Looking it shows that request.user is a SimpleLazyObject and it is a <lambda>, so it can not be pickled.

try:
    import cPickle as pickle
except:
    import pickle


def some_view(request):
    pickle.dumps(request.user) # raise type error
    …

Attachments (2)

ticket_16563.diff (1.6 KB) - added by lukeplant 2 years ago.
Patch with test
ticket_16563.2.diff (1.5 KB) - added by lukeplant 2 years ago.
Small fix to patch (unneeded import)

Download all attachments as: .zip

Change History (25)

comment:1 Changed 3 years ago by PaulM

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

Some things aren't pickleable. It's a known limitation at the Python level, and is unlikely to change. Is Django itself trying to do this somewhere? Is there an extremely good reason this should be possible? I'm closing this as wontfix. Please feel free to reply if you've got good answers to these questions.

comment:2 Changed 3 years ago by SmileyChris

  • milestone set to 1.4
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Design decision needed

I'm going to reopen for now due to the fact this is a regression in 1.4.

Previously, this was possible:

def my_view(request):
    obj = MyObj(user=request.user, amount=1)
    request.session['unsaved_obj'] = obj
    # ...

Now request.user has been changed to a SimpleLazyObject rather than changing the request class to actually lazily return the true user object, this will choke.
Maybe this is an acceptable regression, but it's something I tripped up on in a project recently.

comment:3 Changed 3 years ago by russellm

  • Severity changed from Normal to Release blocker

If this is a regression, then it should be tracked as a release blocker.

comment:4 Changed 3 years ago by jezdez

  • Description modified (diff)

comment:5 Changed 3 years ago by bronger

  • Cc bronger added

comment:6 Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:7 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:8 Changed 2 years ago by anonymous

Same problem with Django 1.4 (trunk), but in Django 1.3 all fine

from django.core.cache import cache
car = Car.objects.get(pk=8809)
car.name = 'test'
car.save()
cache.set('test', request.user, 1)

Raise error:
can't pickle function objects
C:\Python\lib\site-packages\django\core\cache\backends\locmem.py in set

self._set(key, pickle.dumps(value), timeout)

C:\Python\lib\copy_reg.py in _reduce_ex

raise TypeError, "can't pickle %s objects" % base.name

comment:9 Changed 2 years ago by PaulM

Is this regression related to the change to using pickle.HIGHEST_PROTOCOL throughout Django? The memcached backend used that, but locmem didn't. Does that mean this isn't a regression after all, and depended on the non-canonical behavior of locmem using the lowest pickle protocol?

comment:10 Changed 2 years ago by SmileyChris

No, it's due to the fact that a SimpleLazyObject isn't picklable.

comment:11 Changed 2 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from reopened to new

comment:12 Changed 2 years ago by tobias

  • Owner changed from tobias to nobody

comment:13 Changed 2 years ago by poirier

I wouldn't call this a regression unless Django promised you could do it in 1.3, at least implicitly. Not every behavior change is a regression.

comment:14 follow-up: Changed 2 years ago by poirier

FYI, the current code that makes request.user a SimpleLazyObject is from changeset [16305], which was a re-working of [16297], to fix #15929.

A comment cleaned up along the way said:

    # If we access request.user, request.session is accessed, which results in 
    # 'Vary: Cookie' being sent in every request that uses this context 
    # processor, which can easily be every request on a site if 
    # TEMPLATE_CONTEXT_PROCESSORS has this context processor added.  This kills 
    # the ability to cache.  So, we carefully ensure these attributes are lazy. 

which seems like a pretty good reason for making request.user lazy.

Would it be possible to fix this instead by fixing the chain of events somewhere else? e.g. should any access of request.session result in setting the Vary: Cookie header?

comment:15 follow-up: Changed 2 years ago by tobias

As a work around, can't you just pickle a real User model object instead of request.user? E.g., the dumb way to do this might look something like:

def some_view(request):
    pickle.dumps(User.objects.get(pk=request.user.pk))

comment:16 in reply to: ↑ 15 ; follow-up: Changed 2 years ago by poirier

Replying to tobias:

As a work around, can't you just pickle a real User model object instead of request.user?

That would still require a code change when upgrading to 1.4, right?

comment:17 in reply to: ↑ 16 Changed 2 years ago by tobias

Replying to poirier:

Replying to tobias:

As a work around, can't you just pickle a real User model object instead of request.user?

That would still require a code change when upgrading to 1.4, right?

Yes, I meant it as a complement to your original argument. As you said, being able to pickle request.user directly was never documented, so whether or not it's a release blocking regression is debatable.

comment:18 in reply to: ↑ 14 Changed 2 years ago by carljm

Replying to poirier:

Would it be possible to fix this instead by fixing the chain of events somewhere else?

No, I don't think so.

e.g. should any access of request.session result in setting the Vary: Cookie header?

Yes, it should. Any access of the session means the response you are generating is almost certainly dependent in some way on values in the session, which means serving that same response as a cached response to other users would be at best wrong, and at worst a security issue. This applies even more strongly, if anything, to accessing request.user in particular. So it's quite important that request.user remain lazy, and that accessing it trigger Vary: Cookie on the response.

comment:19 Changed 2 years ago by kmike

  • Cc kmike84@… added

Are there obstacles for making LazyObject instances picklable? E.g.:

class LazyObject(object):
    # ...

    def __getstate__(self):
        if self._wrapped is None: 
            self._setup()
        return self._wrapped

    def __setstate__(self, state):
        self._wrapped = state

Changed 2 years ago by lukeplant

Patch with test

comment:20 Changed 2 years ago by lukeplant

@kmike: That doesn't quite work, but it is close. My attached patch works in some cases, if someone can verify that it fixes the issue with a real User instance in environment showing the bug, I will commit it.

I added the code to SimpleLazyObject not LazyObject, to minimize impact with other subclasses of LazyObject. SimpleLazyObject is not designed to be subclassed really - you are supposed to use it just by passing a callable to the constructor.

There is one potential backwards compatibility concern here: if you had used a SimpleLazyObject with a callable that was itself pickleable, it would previously have been possible to pickle that SimpleLazyObject without the patch. Potentially, the object could have been pickled in the 'lazy' state, without the callable having been called.

However, I've tested this, and it seems that - for whatever reason - calling pickle.dumps causes the the callable to be called. Therefore, the object is not serialised in a 'lazy' state - it is always 'evaluated'. And this is just the same as it currently is, except we have made it explicit. The only difference now is that SimpleLazyObject._setupfunc is never available now after unpickling, but that shouldn't be a problem since it is never used.

I'm therefore fairly confident that this patch shouldn't cause other problems.

Version 0, edited 2 years ago by lukeplant (next)

Changed 2 years ago by lukeplant

Small fix to patch (unneeded import)

comment:21 Changed 2 years ago by lukeplant

OK, this has turned out to be rather tricky.

The previous patch only works for objects that have no state. The basic problem is that due to proxying the __class__ attribute, the pickling machinery is confused (I think), and so it stores a reference to the class of the wrapped object, rather than SimpleLazyObject.

So, we need to define the __reduce__ method for full customization, rather than the simpler __getstate__ method. However, for some reason, the presence of the __class__ attribute proxying causes this to fail - if you define __reduce__ on SimpleLazyObject, it is never called when pickling unless you remove the __class__ hacking.

This takes us back to using __getstate__ (which, for some reason, doesn't suffer from the same problem as __reduce__). But now we have to cooperate with the fact that pickling will skip SimpleLazyObject and store a reference to the wrapped class, and define __getstate__ so that it returns the state from the wrapped object. This appears to work:

    def __getstate__(self):
        if self._wrapped is empty:
            self._setup()
        return self._wrapped.__dict__

The result is that on unpickling, the SimpleLazyObject wrapper disappears entirely, which should be OK.

I still get failure if SimpleLazyObject is wrapping a builtin, and nothing but removing the __class__ proxying appears to be able to fix that. But that bug has always been there, and this change doesn't affect it.

I've confirmed that with this patch you can pickle request.user, and on unpickling get an object that compares equal to the original, so I'm therefore committing my improved patch.

Last edited 2 years ago by lukeplant (previous) (diff)

comment:22 Changed 2 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

In [17202]:

Fixed #16563 - Error pickling request.user

Thanks to zero.fuxor for the report

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.