Opened 3 years ago
Closed 3 years ago
#34173 closed Cleanup/optimization (fixed)
SessionMiddleware support 503 status code
| Reported by: | SessionIssue | Owned by: | Abhinav Yadav |
|---|---|---|---|
| Component: | contrib.sessions | Version: | 4.1 |
| Severity: | Normal | Keywords: | |
| Cc: | Florian Apolloner, Anssi Kääriäinen | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
Hi guys,
I have the following situation. In one of my applications I'm having an issue with returning the right status code.
For example I had this situation where I wanted to list 1000 results, this normally takes a couple of seconds, but during this request, my DB went offline or got stuck for some reason. Currently, this resulted in a 500 status code.
In the API client that interfaces with this code we want to return a 503 because of an external source that only retries tasks on specific status codes (like 503), The current SessionMiddleware hijacks the statuscode and makes it impossible to raise a Service Unavailable (503).
if response.status_code != 500:
try:
request.session.save()
except UpdateError:
raise SessionInterrupted(
"The request's session was deleted before the "
"request completed. The user may have logged "
"out in a concurrent request, for example."
)
response.set_cookie(
settings.SESSION_COOKIE_NAME,
request.session.session_key, max_age=max_age,
expires=expires, domain=settings.SESSION_COOKIE_DOMAIN,
path=settings.SESSION_COOKIE_PATH,
secure=settings.SESSION_COOKIE_SECURE or None,
httponly=settings.SESSION_COOKIE_HTTPONLY or None,
samesite=settings.SESSION_COOKIE_SAMESITE,
)
As my DB is offline, this results in a 400 error, as the session can't be saved. But this is incorrect, as the base request isn't a bad request.
I rewrote this small piece in a custom middleware that inherits the SessionMiddleware, but this is not a futureproof solution:
**if response.status_code not in [500, 503]:**
try:
request.session.save()
except UpdateError:
raise SessionInterrupted(
"The request's session was deleted before the "
"request completed. The user may have logged "
"out in a concurrent request, for example."
)
response.set_cookie(
settings.SESSION_COOKIE_NAME,
request.session.session_key, max_age=max_age,
expires=expires, domain=settings.SESSION_COOKIE_DOMAIN,
path=settings.SESSION_COOKIE_PATH,
secure=settings.SESSION_COOKIE_SECURE or None,
httponly=settings.SESSION_COOKIE_HTTPONLY or None,
samesite=settings.SESSION_COOKIE_SAMESITE,
)
It's a small change, but it will make it hard for us to keep track of all the Django updates.
Do you have a solution for this issue?
Thanks in advance.
Change History (11)
comment:1 by , 3 years ago
| Resolution: | → invalid |
|---|---|
| Status: | new → closed |
comment:2 by , 3 years ago
| Description: | modified (diff) |
|---|---|
| Resolution: | invalid |
| Status: | closed → new |
| Summary: | SessionMiddleware only returns 400 or 500 error in case of DB issues. → SessionMiddleware support 503 status code |
| Type: | Bug → New feature |
follow-up: 4 comment:3 by , 3 years ago
| Cc: | added |
|---|
I'm not sure why we should treat 503 differently. IMO, we could reconsider avoiding session saving when status code > 500 🤔
PS. Do not reopen tickets without further explanation.
comment:4 by , 3 years ago
Replying to Mariusz Felisiak:
I'm not sure why we should treat 503 differently. IMO, we could reconsider avoiding session saving when status code > 500 🤔
PS. Do not reopen tickets without further explanation.
That would fix the problem and seems like a convenient solution to me.
Thanks a lot! 😁
follow-up: 8 comment:5 by , 3 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|---|
| Type: | New feature → Cleanup/optimization |
Happy to look at a patch (with tests) adjusting the check to if response.status_code >= 500:
comment:6 by , 3 years ago
| Easy pickings: | set |
|---|
comment:7 by , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:8 by , 3 years ago
Replying to Carlton Gibson:
Happy to look at a patch (with tests) adjusting the check to
if response.status_code >= 500:
Is this a patch we can expect to see in 3.2 (LTS), or will this only be patched in the upcoming 4.2 LTS?
Thanks in advance.
comment:10 by , 3 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Thanks for this ticket, however it is a support question and Trac is not a support channel.