Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing _closable_objects for pickling breaks response processing #1850

Closed
chibisov opened this issue Sep 8, 2014 · 61 comments
Closed

Removing _closable_objects for pickling breaks response processing #1850

chibisov opened this issue Sep 8, 2014 · 61 comments
Labels
Milestone

Comments

@chibisov
Copy link

chibisov commented Sep 8, 2014

Here _closable_objects was added to rendering_attrs 59d0a03#diff-21a03cd19b8422c334c33ec8da66e9c8R21

Every rendering attribute is deleted in __getstate__ method https://github.com/django/django/blob/1.7/django/template/response.py#L45

But _closable_objects is used in base response handler:
https://github.com/django/django/blob/1.7/django/core/handlers/base.py#L210

That's why after processing response from cache i get error like this:

Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/wsgiref/handlers.py", line 85, in run
    self.result = application(self.environ, self.start_response)
  File "/Users/web-chib/drf-ussie/lib/python2.7/site-packages/django/contrib/staticfiles/handlers.py", line 64, in __call__
    return self.application(environ, start_response)
  File "/Users/web-chib/drf-ussie/lib/python2.7/site-packages/django/core/handlers/wsgi.py", line 187, in __call__
    response = self.get_response(request)
  File "/Users/web-chib/drf-ussie/lib/python2.7/site-packages/django/core/handlers/base.py", line 211, in get_response
    response._closable_objects.append(request)
AttributeError: 'Response' object has no attribute '_closable_objects'

I don't know, should i recreate this attribute by hand, or that's DRF error?

@tomchristie
Copy link
Member

tomchristie commented Sep 8, 2014

Want to try to track back the PR and conversation for originally adding it into REST framework in the first place. Personally I think the whole 'pickle'-for-caching responses is butt-ugly, but we just need to find whatever works most consistently with that.

@tomchristie
Copy link
Member

tomchristie commented Sep 11, 2014

Looks to be a bug - text here states to remove this once we don't support 1.3... https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/response.py#L19

We don't support 1.3 now so those lines can be removed - feel free to go ahead and issue a pull request doing so.

In any case those lines shouldn't be there - they should have been in compat.py.

@tomchristie
Copy link
Member

tomchristie commented Sep 11, 2014

Original issue was this: #1747.

@upmauro
Copy link

upmauro commented Sep 14, 2014

👍

@tomchristie
Copy link
Member

tomchristie commented Sep 14, 2014

@upmauro seeing the issue as well?

@upmauro
Copy link

upmauro commented Sep 14, 2014

I saw this problem last night when try to use caching in django 1.7.

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 15, 2014

Adding the _closable_objects in the rendering_attrs was required to make the request pickable in order to support the throttling [EDIT: it is the caching that is failing and not the throttling] under Django 1.7

Before that we had:

_____________________________________________________________________________________ CacheRenderTest.test_get_caching _____________________________________________________________________________________
tests/test_renderers.py:682: in test_get_caching
>       cache.set(self.cache_key, resp)
../../../.virtualenvs/django-rest-framework/lib/python2.7/site-packages/django/core/cache/backends/locmem.py:67: in set
>       pickled = pickle.dumps(value, pickle.HIGHEST_PROTOCOL)
E       PicklingError: Can't pickle <type 'function'>: attribute lookup __builtin__.function failed
____________________________________________________________________________________ CacheRenderTest.test_head_caching _____________________________________________________________________________________
tests/test_renderers.py:672: in test_head_caching
>       cache.set(self.cache_key, resp)
../../../.virtualenvs/django-rest-framework/lib/python2.7/site-packages/django/core/cache/backends/locmem.py:67: in set
>       pickled = pickle.dumps(value, pickle.HIGHEST_PROTOCOL)
E       PicklingError: Can't pickle <type 'function'>: attribute lookup __builtin__.function failed
____________________________________________________________________________________ CacheRenderTest.test_obj_pickling _____________________________________________________________________________________
tests/test_renderers.py:660: in test_obj_pickling
>       self.assertEqual(self._get_pickling_errors(resp), {})
E       AssertionError: {'_closable_objects': None} != {}
E       - {'_closable_objects': None}
E       + {}

@tomchristie the removal is supposed to be for the condition on Django being >= 1.4

I'm a bit lost at what's failing with the cache. Is the entire response cached ?

@tomchristie
Copy link
Member

tomchristie commented Sep 15, 2014

was required to make the request pickable in order to support the throttling under Django 1.7

What, why and where?

We don't use pickle in any of our throttling implementations. How come we're tweaking a bit of private API? What fails if we don't do this, and why.

Aside: Cursing at the hideousness of pickle as a design idea in general and at Django's caching middleware specfically for using it on response classes instead of just storing the response headers and body in the cache and recreating the object. Yuck.

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 15, 2014

@tomchristie my bad, I edited my post because it was caching issue, not a throttling one.
I'm trying to figure how this change had affected the cache.
__getstate__ doesn't affect the overall Response since HttpResponseBase does a copy of the dict. Moreover the _closable_objects are transient objects that needs to be closed at the end of the request which means it doesn't make sense to cache them.

Can anyone give more context about when the issue does happen ?
Is it when you are trying to cache the response ? Or maybe when the response is already in cache and returned from there ?

@tomchristie
Copy link
Member

tomchristie commented Sep 15, 2014

Two things here:

  1. We need to replicate this issue so we don't bump into it again.
  2. In any case we ought to reconsider why we have this _closable_objects objects hack in the first place - does not seem like something we should be having to alter. To confirm: do any tests fail when it's removed?

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 15, 2014

@tomchristie The py.test above was with current master with the _closable_objects removed.
I will dig this issue further tonight. I need to figure out whether response caching does work correctly with Django 1.7 and if they really do understand why they don't out of the box with rest framework. This should lead to a test case against this issue.

@tomchristie
Copy link
Member

tomchristie commented Sep 15, 2014

Also possible that our tests are incorrect. We might instead want to be testing that our responses work okay together with Django's caching middleware, rather than testing against the low-level pickle API directly.

@upmauro
Copy link

upmauro commented Sep 15, 2014

Guys, i'm beginner and i'm not good to speak English, but i will try explain what happens with me.

When i append,

'django.middleware.cache.UpdateCacheMiddleware',
'django.middleware.cache.FetchFromCacheMiddleware',

In MIDDLEWARE_CLASSES and use this in my views:

@cache_page(60 * 15)
class VeiculoMarcaViewSet(viewsets.ModelViewSet):

The exception raised:

NoReverseMatch at /api/
Reverse for 'veiculomarca-list' with arguments '()' and keyword arguments '{}' not found. 0 pattern(s) tried: []

But in sometimes when i restart my sever and clean memcache, show AttributeError: 'Response' object has no attribute '_closable_objects'. I'm crazy?

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 15, 2014

@upmauro thanks. That's exactly what I was looking for. This will help us reproduce the issue.

@upmauro
Copy link

upmauro commented Sep 15, 2014

@xordoquy great! if you need more informations, please tell-me

@ondrowan
Copy link

ondrowan commented Sep 15, 2014

@xordoquy chibisov/drf-extensions#32 - this is my original problem (read also comments for further info).

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 15, 2014

@ondrowan thanks

@upmauro
Copy link

upmauro commented Sep 16, 2014

Any ideas? For now downgrade solve ?

@upmauro
Copy link

upmauro commented Sep 16, 2014

I find the problem in core, but i can't solve :(, someone can help-me ?

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 16, 2014

@upmauro downgrade should solve this. Meanwhile I'll be working on this a bit more today

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 16, 2014

By the way, adding the cache middleware makes the test suit fail heavily whether or not _closable_objects is set in the rendering_attrs

@picturedots
Copy link

picturedots commented Sep 16, 2014

After upgrading DRF to 2.4.2 I hit this problem with Django 1.5.10 and my own caching code (not using django.middleware.cache). Workaround as suggested in DRF extensions github repo seems to be working for me, which is to explictly add _closable_objects to request AFTER fetching the response from the catch. Something like

    response = cache.get(cache_key)
    if not hasattr(response, '_closable_objects'):
        response._closable_objects = []     
    return response

@tomchristie
Copy link
Member

tomchristie commented Sep 16, 2014

Anyone want to figure out what part of the response object it's trying and failing to pickle, that's responsible for raising the error?

/aside again. grumble grumble pickling entire arbitrary response object awful idea in the first place grumble grumble.

@upmauro
Copy link

upmauro commented Sep 16, 2014

About my problem i oppened another issue #1875 because i dont know if the same.

@Suor
Copy link

Suor commented Dec 15, 2016

Why is this closed? Picking-unpickling should not break the object.

@tomchristie
Copy link
Member

tomchristie commented Dec 15, 2016

Because no-one's demonstrated that there's a regression. Looks feasible that it needs reopening, but let's start with a minimal reproducible case and take it from there.

ioparaskev added a commit to grnet/flowspy that referenced this issue Nov 15, 2018
Summary:
- Removes DjangoModelPermissions from the settings template.
DjangoModelPermissions is not needed since the user actions are
validated based on their organization allowed peers. There is no other
permission model/group. DjangoModelPermissions rendered the non-GET
actions unusable until now in the api (only admins could make such
actions) whereas in the views, such actions were allowed
(because the view used django forms instead of api callbacks)
- Downgrades djangorestframework version to solve #1850 drf bug with
pickling and caching. Bug was solved after version 3.0.4 which until now
is not tested, so it's safer to downgrade. More info about the bug:
encode/django-rest-framework#1850
- Fix documentation example for `then` rule
- Adds PENDING in allowed Route statuses since the rules are applied
asynchronously and this means that until applied, the Route will have to
be in PENDING status. This was stated in commit
11171aa message but was not implemented

Reviewers: #developers, zmousm

Reviewed By: #developers, zmousm

Subscribers: zmousm, #developers

Differential Revision: https://phab.noc.grnet.gr/D5182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests