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

Use import_string for token_backend (Fixes #434) #435

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Conversation

Andrew-Chen-Wang
Copy link
Member

Fixes #434 hopefully @dabljues

Please let me know if this resolves your issue. You can test this by doing pip install git+https://github.com/jazzband/djangorestframework-simplejwt.git@token-backend

Do not merge immediately. There are performance improvements that we can possibly make here such as initially setting this in the settings rather than using state.py

@dabljues
Copy link

@Andrew-Chen-Wang

I can confirm this solves the issue, I tried multiple scenarios including the one from #434 and the issue doesn't appear.

dabljues added a commit to dabljues/project_manager that referenced this pull request Jul 15, 2021
Fix pending:
jazzband/djangorestframework-simplejwt#435.
After a new version with the fix gets released, bump the minimal
requirement for the package.
@Andrew-Chen-Wang
Copy link
Member Author

@dabljues Fantastic! I'd like to see if there's more things I could try out to improve performance (since every time a new token is used, we have to reimport the token backend).

And when it comes to users constantly hitting your refresh view, the performance degradation could be massive. So I'll need to do some performance testing before this is merged, and I may need to ping you in a couple other PRs to see if your scenarios result in any circular imports if you don't mind.

Thank you!

@dabljues
Copy link

Not a problem, will try to stay available :)

@Andrew-Chen-Wang
Copy link
Member Author

tl;dr LGTM

Just did a quick timing test:

In [1]: 
   ...: from django.utils.module_loading import import_string
   ...: 
   ...: 
   ...: class Blah:
   ...:     def hi(self):
   ...:         from rest_framework_simplejwt.state import token_backend
   ...: 
   ...: 
   ...: class Blah1:
   ...:     def hi(self):
   ...:         backend = import_string("rest_framework_simplejwt.state.token_ba
   ...: ckend")
   ...: 
   ...: 
   ...: def test():
   ...:     obj = Blah()
   ...:     obj.hi()
   ...: 
   ...: 
   ...: def test1():
   ...:     obj = Blah1()
   ...:     obj.hi()
   ...: 

In [2]: %timeit for x in range(100): test()
111 µs ± 5.31 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [3]: %timeit for x in range(100): test1()
1.41 ms ± 151 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [4]: %timeit for x in range(1000): test()
1.19 ms ± 76.9 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [5]: %timeit for x in range(1000): test1()
13.3 ms ± 815 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Note that I did not use the _token_backend class attribute for saving the value in memory. However, looking back in history as to why we have our backend stored in the state.py file, it looks like holding the value in class attribute memory is OK since it shouldn't be modified -- historically, at least, that's what it looks like.

@Andrew-Chen-Wang
Copy link
Member Author

@auvipy thanks for helping to review/approve these PRs from me!

@auvipy auvipy merged commit ff59830 into master Aug 12, 2021
@auvipy auvipy deleted the token-backend branch August 12, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circular import
4 participants