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

Avoid unnecessary user get expiring session memberships #3910

Merged
merged 1 commit into from
May 20, 2019

Conversation

AlanCoding
Copy link
Member

SUMMARY

This is something to speed up our middleware by 1 database query for every request.

ISSUE TYPE
  • Performance
COMPONENT NAME
  • API
AWX VERSION
4.0.0
ADDITIONAL INFORMATION

there seems to be no need to obtain the object at all, so we can just not do it. I tested the filter and create commands via the shell manually, and I know we have at least 1 functional unit test that covers this code path.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@AlanCoding
Copy link
Member Author

recheck

@AlanCoding AlanCoding requested a review from rooftopcellist May 16, 2019 18:35
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Member

@rooftopcellist rooftopcellist left a comment

Choose a reason for hiding this comment

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

lgtm

@AlanCoding
Copy link
Member Author

I have a gut feeling that I should rebase and test this with #3875 before merging it, so I will call it blocked until that lands. This makes the most sense in terms of simple merge order.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member Author

Also want to test with rebase on top of #3918 and #3890

@AlanCoding
Copy link
Member Author

This is kind of the gist of the answer I was looking for:

/venv/awx/lib64/python3.6/site-packages/django/contrib/sessions/middleware.py in process_response(58)
  request.session.save()
/venv/awx/lib64/python3.6/site-packages/django/contrib/sessions/backends/db.py in save(87)
  obj.save(force_insert=must_create, force_update=not must_create, using=using)
/venv/awx/lib64/python3.6/site-packages/django/dispatch/dispatcher.py in send(193)
  for receiver in self._live_receivers(sender)
/venv/awx/lib64/python3.6/site-packages/django/dispatch/dispatcher.py in <listcomp>(193)
  for receiver in self._live_receivers(sender)
./awx/main/signals.py in save_user_session_membership(658)
  if UserSessionMembership.objects.filter(user=user_id, session=session).exists():

I just wanted to have a sense of if and where this gets called from after the reordering. It seems that it is called from SessionMiddleware

https://github.com/django/django/blob/1.11.19/django/contrib/sessions/middleware.py#L58

Everything should be perfectly okay with that.

This is a post-save signal, and we are able to clearly identify the original save that happens

/venv/awx/lib64/python3.6/site-packages/django/contrib/sessions/middleware.py in process_response(58)
  request.session.save()
/venv/awx/lib64/python3.6/site-packages/django/contrib/sessions/backends/db.py in save(87)
  obj.save(force_insert=must_create, force_update=not must_create, using=using)

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit dc1bf3e into ansible:devel May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants