When settings.PASSWORD_HASHERS is changed and user.check_password() is called in an async context, a SynchronousOnlyOperation exception may occur.
The reason is that the check_password function will call the synchronous setter function to update the password field of the user table when the settings.PASSWORD_HASHERS change.
I think this exception can be solved by defining async acheck_password and defining async asetter function in acheck_password. So that in an async context, user.acheck_password() can be called to avoid the above exception.
If this solution is feasible, I am willing to contribute code.
But… we need to think about what we need to support being async. One answer is everything, but I wonder if we really need changing passwords to be done with async? I’d be inclined to just use the sync views and methods for that, so explaining the use-case, the why would help.
I would like to explain my views on asynchrony. I think the main benefit of async is that io will no longer block the thread, so it can improve Django’s performance.
Therefore, if we say what interfaces we need to asynchronize, I think the answer is that most of the interfaces that involve io, unless the cost of asynchronizing some interfaces is very high, they should be asynchronized.
Another thing is about why AbstractBaseUser.check_password() needs to be asynchronized. I think what you said makes sense, if we simply change the password, then we can do it in a synchronized view. But in the default AUTHENTICATION_BACKENDS, namely ModelBackend, check_password() will be called in the authenticate function. If PASSWORD_HASHERS happens to be modified in an asynchronous context, an exception may be raised.
In fact, in Django ticket #39102, in order to avoid this exception, the sync_to_async method is used to wrap auth.get_user. But I think using sync_to_async is only a temporary solution, because auth.get_user is a very frequently called function, and the asynchronous implementation using threads is not as high as native asynchronous performance after all, so we should eventually implement a native asynchronous version for auth.get_user.
If that’s what I said, it would make sense to add asynchronous support to AbstractBaseUser.check_password(), since this is the first step towards a natively asynchronous auth.get_user.
If a user’s password hasher is different from the currently configured preferred password hasher, it may trigger the setter function of check_password to update password. The setter of check_password does not work properly in an asynchronous context. To avoid this problem, auth.get_user() needs to be decorated with sync_to_async.
But I think that using sync_to_async to decorate auth.get_user() is not a long-term solution. For better performance, we should implement the asynchronous version of auth.get_user()
I think coordinating with @bigfootjon’s work is the way forward here. There’s an ordering of work that makes sense, and he’s already hit some of the barriers.
I leave it open whether the whole stack eventually needs to be async or not: performance gains for low throughput pathways (like password checks) are more theoretical than real, so it could be that we never need to make this async… (Note only “could” — the point is more to focus work on things are going to need first first, regardless of a potential end-state.)
However, all effort is appreciated here! It’s nice to have another active contributor in the async space. There’s lots to do.
Hey y’all. I’ll follow up on the thread I started, but just to make some points inline:
But… we need to think about what we need to support being async. One answer is everything , but I wonder if we really need changing passwords to be done with async?
I can imagine a scenario where changing passwords is one of many I/O tasks that need to be completed, and using async can speed things up. For example, in a hypothetical case where the hasher was changed and all users need to upgrade when logged in. I might want to bump some count on a logging system that another user has upgraded their hash & also save the changes to the database. This is a perfect case for async IMO.
Therefore, if we say what interfaces we need to asynchronize, I think the answer is that most of the interfaces that involve io, unless the cost of asynchronizing some interfaces is very high, they should be asynchronized…
I agree with everything in this message, I couldn’t have said it better.
I leave it open whether the whole stack eventually needs to be async or not: performance gains for low throughput pathways (like password checks) are more theoretical than real, so it could be that we never need to make this async… (Note only “could” — the point is more to focus work on things are going to need first first, regardless of a potential end-state.)
I’ll make a note to myself to convince you when the time comes
Great. I know he hit difficulties, but I’m not sure what the state of play is without going back to look at the PR/tickets.
I’m personally happy with the current solution, I think it’s ready when you / @felixxm have a spare minute for another round of reviews.
If you check then flags on Trac are set correctly (uncheck the Patch needs improvement for example) it’ll show up on the Patches needing review list, and they’ll get to it. (I’m on retirement detox at the moment so am deliberately not looking for the moment. )
It is indeed possible to solve the Synchronous only operation exception by defining an asynchronous check password function and an asynchronous setter function in acheck password. By doing so, you can call user.acheck_password() in an async contect, avoiding the exception. If you believe this solution is feasible and you are willing to contribute code, you can proceed by implementing the async versions of the check password and setter functions. Make sure to thoroughly test the changes and consider the impact on the overall system before submitting the code contribution.
Thank you for your reply. In order to improve the PR pass rate, I am afraid that I will not take actual actions until this proposal has more support from the community.
If you need this functionality, please have a look under this repository. This is the initial implementation of acheck_password and its test cases
Hey @HappyDingning, can you clarify what you mean here? As far as I can tell from reading this thread this idea has plenty of support, it certainly has mine! The commits on your repository look to be in really good shape, I think you could create a Pull Request with those commits.
The only missing detail is that you need to open a ticket and get it accepted before creating the Pull Request. Go here to create one: https://code.djangoproject.com/ (you can log in with your GitHub account).
Add myself (Jon Janzen) and @carltongibson (Carlton Gibson) to the “CC” section of the ticket. Also a good idea to add “async” to the “keywords” section of the ticket.
Assuming someone accepts the ticket you can create a Pull Request. It’ll possibly be a few rounds of review on the PR before it is accepted, but the commits you have now look like a great start!
Thank you for your reply. My main concern is that the ticket will not be accepted. But after hearing what you said, I think I can give it a try. I’ll tidy up the code soon and try to create a ticket.
)