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.

Sign up for GitHub

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

Jump to bottom

Fixed #31962 -- Made SessionMiddleware raise SessionInterrupted when session destroyed while request is processing. #13395

Merged
merged 2 commits into from Sep 9, 2020
Merged

Fixed #31962 -- Made SessionMiddleware raise SessionInterrupted when session destroyed while request is processing. #13395

merged 2 commits into from Sep 9, 2020

Conversation

hramezani
Copy link
Member

@hramezani hramezani force-pushed the ticket_31962 branch 2 times, most recently from b8e6e0f to 8ff6eb4 Compare September 7, 2020 12:24
@hramezani
Copy link
Member Author

Here is the docs test error:
WARNING: py:exc reference target not found: django.contrib.sessions.exceptions.SessionIntrupted

Don't know how to fix it 😞

@timgraham
Copy link
Member

The error is because of a typo: "Intrupted".

@hramezani
Copy link
Member Author

The error is because of a typo: "Intrupted".

Thanks @timgraham, and sorry for that 🤦‍♂️

felixxm
felixxm reviewed Sep 8, 2020
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hramezani Thanks for this patch 👍 I pushed edits to docs. This exception should trigger a 400 response (see comment), currently it's handled as a 500. We also a need test for this.

docs/ref/exceptions.txt Outdated Show resolved Hide resolved
@hramezani
Copy link
Member Author

Thanks @felixxm for the review and edit!

This exception should trigger a 400 response

How can I handle this exception as a HttpResponseBadRequest?

@felixxm
Copy link
Member

felixxm commented Sep 8, 2020

How can I handle this exception as a HttpResponseBadRequest?

That's a good question. Maybe we can add a new core exception e.g. BadRequest (and SessionInterrupted as its subclass) which will be handled in response_for_exception(), not sure.

@hramezani hramezani force-pushed the ticket_31962 branch 3 times, most recently from ca91fb3 to f512100 Compare September 8, 2020 13:06
hramezani
hramezani commented Sep 8, 2020
View reviewed changes
django/core/handlers/exception.py Outdated
@@ -76,6 +76,9 @@ def response_for_exception(request, exc):
exc_info=sys.exc_info(),
)

elif isinstance(exc, BadRequest):
response = get_exception_response(request, get_resolver(get_urlconf()), 400, exc)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm Do we need to use log_response here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should also return technical 500 page when DEBUG is True. I fixed this.

@hramezani
Copy link
Member Author

How can I handle this exception as a HttpResponseBadRequest?

That's a good question. Maybe we can add a new core exception e.g. BadRequest (and SessionInterrupted as its subclass) which will be handled in response_for_exception(), not sure.

I added BadRequest and made SessionInterrupted subclass of it.
I am not sure that I did the test in the right way.

@felixxm felixxm changed the title Fixed #31962 -- Made sessions middleware raise SessionInterrupted exception when session cannot be updated. Fixed #31962 -- Made SessionMiddleware raise SessionInterrupted when session destroyed while request is processing. Sep 9, 2020
felixxm
felixxm reviewed Sep 9, 2020
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hramezani Thanks 👍

django/core/handlers/exception.py Outdated
@@ -76,6 +76,9 @@ def response_for_exception(request, exc):
exc_info=sys.exc_info(),
)

elif isinstance(exc, BadRequest):
response = get_exception_response(request, get_resolver(get_urlconf()), 400, exc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should also return technical 500 page when DEBUG is True. I fixed this.

django/core/exceptions.py Outdated Show resolved Hide resolved
tests/sessions_tests/tests.py Outdated Show resolved Hide resolved
carltongibson
carltongibson approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

I had a slight worry about the status code… 400 the request is malformed… feels like the request is fine but it's unauthorized (the exact same request would succeed otherwise). HOWEVER I think 400 is probably the right way to go.

These kind of timing things are difficult. Makes you glad for ATOMIC_REQUESTS.

Thanks @hramezani an @felixxm

django/core/exceptions.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Sep 9, 2020

Thanks Carlton!

@felixxm felixxm merged commit 4539674 into django:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixxm felixxm felixxm left review comments

@carltongibson carltongibson carltongibson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@hramezani @timgraham @felixxm @carltongibson