Fixed #29186 -- Fixed pickling HttpRequest and subclasses.

[Anv3sh]

Introduced DjangoSocketHandler class to handle socket logging which simply removes all the non-picklable attributes from the request object.

[felixxm]

@Anv3sh Do you have time to keep working on this?

[Anv3sh]

@Anv3sh Do you have time to keep working on this?

Yes I'm working on it.

non_picklable_attrs = frozenset(
           [
            "resolver_match",
            "environ",
            "META",
            "_stream",
           ]
   ) 

Couple of things that I'm confused with is that if we keep the environ and META attrs as non-picklable it works fine while socket logging and gets pickled but since __getstate()__ is used by the .as_view() it needs the environ and META and throws exception in some of the testcases.

The HttpRequest.__getstate__( ) looks as following :

     def __getstate__(self):
        obj_dict = self.__dict__.copy()
        for attr in self.non_picklable_attrs:
            if attr in obj_dict:
                del obj_dict[attr]
        return obj_dict

[felixxm]

Couple of things that I'm confused with is that if we keep the environ and META attrs as non-picklable it works fine while socket logging and gets pickled but since __getstate()__ is used by the .as_view() it needs the environ and META and throws exception in some of the testcases.

as_view() doesn't use __getstate__(). Tests errors (e.g. auth_tests.test_templates.AuthTemplateTests.test_password_reset_view) are related with creating request in the setUpTestData() which uses __dict__ and deepcopy() of attributes to ensure isolation.

The first question is: Is the environ really non-picklable? If yes, can we try to override HttpRequest.__deepcopy_()?

[Anv3sh]

Couple of things that I'm confused with is that if we keep the environ and META attrs as non-picklable it works fine while socket logging and gets pickled but since __getstate()__ is used by the .as_view() it needs the environ and META and throws exception in some of the testcases.

as_view() doesn't use __getstate__(). Tests errors (e.g. auth_tests.test_templates.AuthTemplateTests.test_password_reset_view) are related with creating request in the setUpTestData() which uses __dict__ and deepcopy() of attributes to ensure isolation.

The first question is: Is the environ really non-picklable? If yes, can we try to override HttpRequest.__deepcopy_()?

Got it 👍🏻. As far as I tested the environ isn't picklable so as you said we might have to override the HttpRequest.__deepcopy__() but I don't have much idea on how we can proceed with that if you could give some suggestions?

[Anv3sh]

@felixxm overrided the HttpRequest.__deepcopy__() still don't know if we should treat META as a non-picklable attribute as I ran this custom test:

def test_pickle_request(self):
        request = HttpRequest()
        request.method = "GET"
        request.COOKIES = {"post-key": "post-value"}
        dump = pickle.dumps(request)
        request_from_pickle = pickle.loads(dump)
        self.assertEqual(repr(request), repr(request_from_pickle))

I found out that the HttpRequest.__repr__() needs the META.QUERY_STRING attribute but if we treat META as a picklable attribute the socket logging throws error. If you could take a look 🙇‍♂️

[felixxm]

@Anv3sh Please add tests.

[Anv3sh]

@Anv3sh Please add tests.

Can you please review this comment🙇‍♂️

[felixxm]

I found out that the HttpRequest.repr() needs the META.QUERY_STRING attribute but if we treat META as a picklable attribute the socket logging throws error. If you could take a look

So, again, Can you add a test that crashes when META in not in the non_picklable_attrs? then we could discuss what's inside META. As far as I'm aware, META should generally be treated as non-picklable even though it can be picklable in some basic cases.

[Anv3sh]

I found out that the HttpRequest.repr() needs the META.QUERY_STRING attribute but if we treat META as a picklable attribute the socket logging throws error. If you could take a look

So, again, Can you add a test that crashes when META in not in the non_picklable_attrs? then we could discuss what's inside META. As far as I'm aware, META should generally be treated as non-picklable even though it can be picklable in some basic cases.

Ok I will add the test case and remove the META attribute from the non_picklable_attrs for the test to pass and then we can add it later after checking.

[Anv3sh]

Added the test for request pickling and sorry for the linting errors missed checking for them.

[Anv3sh]

Added the META attributes that were non_picklable to META_non_picklable_attrs and changed the test_pickling_request according to that. Still not able to figure out a test for actual requests( WSGIRequest ,ASGIRequest) which crash when removing any key from non_picklable_attrs.

[Anv3sh]

@felixxm added the WSGIRequest.__getstate__() and WSGIRequest.wsgi_non_picklable_attrs

[felixxm]

@Anv3sh Thanks 👍

This patch should allow for removing asgi_request and wsgi_request from HttpResponse.non_picklable_attrs.

[Anv3sh]

@felixxm committed the requested changes don't know why one of the checks failed.

[felixxm]

@Anv3sh Thanks 👍

We don't need more tests because tests added in d7f5bfd will check that WSGIRequest and ASGIRequest are picklable (after removing wsgi_request and asgi_request from HttpResponse.non_picklable_attrs).

@felixxm committed the requested changes don't know why one of the checks failed.

Don't worry this test is flaky.

[vamsi-verma-s]

Hi @felixxm, will this fix be back-ported to older versions? if so, any timeline?

[vamsi-verma-s]

nvm, just read the doc

[salomvary]

I don't really understand what the purpose of this change was, but it broke code in Django 4.2 that tries to "clone" and override requests using copy.

Something like this was working before:

request = HttpRequest()
request_copy = copy(request)
match = request_copy.resolver_match

AttributeError: 'WSGIRequest' object has no attribute 'resolver_match'

[felixxm]

@salomvary Thanks for the report.

Something like this was working before:

It worked only because resolver_match was None. However, I agree that we could make it less backward compatible, e.g.:

diff --git a/django/http/request.py b/django/http/request.py
index 2ef9dfd649..a82851f660 100644
--- a/django/http/request.py
+++ b/django/http/request.py
@@ -87,7 +87,7 @@ class HttpRequest:
     def __getstate__(self):
         obj_dict = self.__dict__.copy()
         for attr in self.non_picklable_attrs:
-            if attr in obj_dict:
+            if obj_dict.get(attr, None) is not None:
                 del obj_dict[attr]
         return obj_dict
 

Please create a new ticket in Trac and follow our bug reporting guidelines. All bugfixes require an accepted ticket.

[salomvary]

Thanks for the quick response!

It worked only because resolver_match was None.

I don't see why shallow copying a HttpRequest with a non-None resolver_match should be a problem. Am I missing something? (I'm not really familiar with the internals of copy and pickle.)

For me it sounds like this was a change that was necessary for pickle to work on HttpRequest which inadvertently affected how copy now behaves.

[felixxm]

Some attributes of HttpRequest and HttpResponse are not picklable (see ticket-32969, ticket-29186, and ticket-23895 for more details and some discussion) which breaks e.g. logging. That's why we decided to strip non-picklable attributes.

[Anv3sh]

@salomvary Thanks for the report.

Something like this was working before:

It worked only because resolver_match was None. However, I agree that we could make it less backward compatible, e.g.:

diff --git a/django/http/request.py b/django/http/request.py
index 2ef9dfd649..a82851f660 100644
--- a/django/http/request.py
+++ b/django/http/request.py
@@ -87,7 +87,7 @@ class HttpRequest:
     def __getstate__(self):
         obj_dict = self.__dict__.copy()
         for attr in self.non_picklable_attrs:
-            if attr in obj_dict:
+            if obj_dict.get(attr, None) is not None:
                 del obj_dict[attr]
         return obj_dict
 

Please create a new ticket in Trac and follow our bug reporting guidelines. All bugfixes require an accepted ticket.

Should I create the required ticket?? 😄

[felixxm]

Should I create the required ticket?? smile

Yes please, I think we should fix cloning an empty HttpRequest and HttpResponse objects with None attributes (regressions in d7f5bfd and 6220c44).

[Anv3sh]

Should I create the required ticket?? smile

Yes please, I think we should fix cloning an empty HttpRequest and HttpResponse objects with None attributes (regressions in d7f5bfd and 6220c44).

Got it 👍🏻