Fixed #34806 -- Made the cached_db backend resilient to cache backend errors.

[sulabhkatila]

ticket_34806

I have added a MemoryError handling to ensure that it doesn't crash and logged the issue to let the user know.

[shangxiao]

Hi & thanks for submitting this PR 😊

There's a change required to the way the logger is created. Also we'll need a new test to confirm the behaviour change.

[shangxiao]

Unfortunately we can't just log willy-nilly, we have to follow the standard process of getting a named logger and logging to that to allow users of the framework to capture those logs.

I'm not sure myself what the appropriate logger is for this module but nessita or felixx will be able to confirm that.

[sulabhkatila]

Thank you for the feedback and guidance.

Once I have the information from @felixxm or @nessita, I'll make the necessary updates to the code accordingly.

[nessita]

I did talk with Mariusz about this, and he confirmed that:

• we should use a new logger for django.contrib.sessions:

import logging

logging.getLogger("django.contrib.sessions")

• we should add a release note about the adding of this logger and a documentation entry in ref/logging

[nessita]

@sulabhkatila Any chance you missed the comment above? I don't see those changes reflected in the current diff.

[shangxiao]

@claudep Is this the correct exception to catch?

[claudep]

I'm not sure we can target any specific exception type. In my use case, it was a TooBig error generated by pylibmc. It looks like the error is defined in https://github.com/lericson/pylibmc/blob/97ace274cffd8db/src/_pylibmcmodule.h#L201C12-L201C12 and raised from C code. I'm afraid we have no real choice than catching the broader Exception base class.

[shangxiao]

There's a change required to the way the logger is created. Also we'll need a new test to confirm the behaviour change.

Oh and there may be some documentation updates as well to let folks know "from x.xx version caching errors will result in logs"… felix & nessita will confirm this though 👍

[nessita]

I don't have the answers to the logger questions but I'll check with @felixxm in our next catch up, and get back to you (in about a week).

In the meantime, any chance that the lint errors are fixed?

[sulabhkatila]

hi @nessita, can you please lead me in the right direction in terms of adding a test case for this?

[shangxiao]

@sulabhkatila Nessita's busy, I can help. You've already added a test? 🤔

[shangxiao]

@sulabhkatila A good starting logger may be django.request. Have a look at the codebase for existing uses, eg logger = getLogger("django.request"). Usually loggers at created at module-level and placed at the top after the imports.

[sulabhkatila]

@sulabhkatila Nessita's busy, I can help. You've already added a test? 🤔

Hi @shangxiao,
I added the test but it actually doesn't really work. I only added that to show where my head was going with the test. So my idea for the test was to try to recreate the error and get the test case to catch any error that would be raised (Thought it would be the way from here).

But it doesn't really work: the test case passes even when we raise the error here instead of passing (how it is done now).

I am now suspicious if I am even catching the exception properly as if there isn't where the exception is raised??

[shangxiao]

Getting better 🎉

[shangxiao]

Optionally we can do logger.exception(<message>) which captures the exception:

https://docs.python.org/3/library/logging.html#logging.Logger.exception

This hasn't been used in Django yet so let's wait to see what Mariusz thinks.

[sulabhkatila]

Hi @shangxiao, i have made the proposed changes and also tried to update the documentation in this commit.

[shangxiao]

Hi @shangxiao, i have made the proposed changes and also tried to update the documentation in this commit.

Thanks 🏆 The logger still isn't at module-level but nessita may be ok with that.

[nessita]

Hi @shangxiao, i have made the proposed changes and also tried to update the documentation in this commit.

Thanks 🏆 The logger still isn't at module-level but nessita may be ok with that.

Thanks for the ping, looking into this now!

[nessita]

There's a change required to the way the logger is created. Also we'll need a new test to confirm the behaviour change.

Oh and there may be some documentation updates as well to let folks know "from x.xx version caching errors will result in logs"… felix & nessita will confirm this though 👍

Yes, I think we do need a release note stating this and the adding of the new logger.

[nessita]

Hi @shangxiao, i have made the proposed changes and also tried to update the documentation in this commit.

Thanks 🏆 The logger still isn't at module-level but nessita may be ok with that.

I am not OK with that :-)

Please let's move the getLogger code to right after the imports, at the module level. Thanks!

[nessita]

Added some comments. Also, please note that the .DS_Store binary file that was added, needs to be removed, it does not belong in the repo.

Thank you!

[sulabhkatila]

Hi @shangxiao, I have made the changes as discussed by moving the logger, update to the docs in the appropriate file, and removed the .DS_Store binary file.

[nessita]

As mentioned in one of the comments, a new logger needs to be defined and documented, and also the call to save() inside the try-except block should be moved outside that block.

Thank you for the work so far!

[nessita]

Hello @sulabhkatila, please read my last comment in full, there are still multiple things pending to implement, such as documenting the new logger and moving the save() call to outside the try-except block.

The documenting of the new logger needs to occur in multiple doc pages, I listed those in previous comments I think.

Please only when all those are ready, mark this PR as ready for review in the issue tracker as documented in https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-review-checklist

[sulabhkatila]

Hi @nessita , I apologize for the oversight.

Regarding the docs, I just added the release note about the behavior in docs/releases/5.0.txt.
Now, the docs related updates have been done on the following files: docs/releases/5.0.txt and docs/topics/cache.txt.
Is there any other pages in the docs that I might need to update?

Regarding the save call, initially I had put super().save(must_create()) call inside the try block, and after your feedback I figured I had to put that save call outside and I made the change. And now it looks as such:

 def save(self, must_create=False):
        super().save(must_create)
        try:
            self._cache.set(self.cache_key, self._session, self.get_expiry_age())
        except UpdateError:
            raise
        except Exception as e:
            logger.error("Error saving to cache: %s", e)

Is there some other save() calls that I were supposed to touch. Can you please provide more feedback on it.

[nessita]

Thank you @sulabhkatila for the updates.

Regarding your question:

Is there any other pages in the docs that I might need to update?


Yes, the logging docs also need updating (see docs/ref/logging.txt). Could you also please rebase onto main so you incorporate the latest changes from the main branch?

[nessita]

Hey @sulabhkatila, thank you for your continued work. If this branch is ready for a re-review, please double check that the ticket flags are properly set or unset. This are the docs describing those details.

[nessita]

@claudep Hi there! I pushed some code and docs tweaks, I think it's ready to be merged. Would you fancy doing a final review to this PR? (no worries if you can't!)

[claudep]

Thanks a lot for working on this! We'll see in the future if other backend operations need some similar safeguards.