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

Convert provider from twisted deferreds to async #5

Closed
anoadragon453 opened this issue Aug 18, 2020 · 12 comments · Fixed by #8
Closed

Convert provider from twisted deferreds to async #5

anoadragon453 opened this issue Aug 18, 2020 · 12 comments · Fixed by #8

Comments

@anoadragon453
Copy link

Someone mentioned in matrix-org/synapse#8108 that they used a very similar implementation of a password provider to the one in this repo, which has started raising exceptions come Synapse v1.19.0. This is due to Synapse phasing out Twisted defer calls, and instead switching to Python async/await (which brings increased performance and ability to profile the codebase).

@menturion updated the codebase according to their comment. The same will likely need to happen here for this provider to work properly with Synapse v1.19.0.

@motey
Copy link

motey commented Sep 9, 2020

yep, prevents me from upgrading to v1.19.x

@motey
Copy link

motey commented Sep 30, 2020

Just tested with synapse v1.20.1
Error log:

2020-09-30 20:01:07,443 - synapse.http.server - 85 - ERROR - POST-39 - Failed handle request via 'LoginRestServlet': <XForwardedForRequest at 0x7fc4180dbc10 method='POST' uri='/_matrix/client/r0/login' clientproto='HTTP/1.1' site=8008>
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
    result = g.send(result)
StopIteration: @motey:myserver.org

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/synapse/http/server.py", line 230, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "/usr/local/lib/python3.7/site-packages/synapse/http/server.py", line 407, in _async_render
    callback_return = await raw_callback_return
  File "/usr/local/lib/python3.7/site-packages/synapse/rest/client/v1/login.py", line 128, in on_POST
    result = await self._do_other_login(login_submission)
  File "/usr/local/lib/python3.7/site-packages/synapse/rest/client/v1/login.py", line 239, in _do_other_login
    identifier["user"], login_submission
  File "/usr/local/lib/python3.7/site-packages/synapse/handlers/auth.py", line 820, in validate_login
    is_valid = await provider.check_password(qualified_user_id, password)
  File "/usr/local/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
    result = g.send(result)
  File "/usr/local/lib/python3.7/site-packages/rest_auth_provider.py", line 122, in check_password
    for threepid in (yield store.user_get_threepids(user_id)):
TypeError: 'coroutine' object is not iterable

BUT yesterday @devplayer0 created a fork with a fix :=) cheers.
devplayer0@893473b
fixed the issue for me.

@anoadragon453 just use the fork https://github.com/devplayer0/matrix-synapse-rest-password-provider

@ma1uta would be great if you could merge the fix into this repo.

@devplayer0
Copy link

I should probably open a PR for this!

Unfortunately I've been really busy and just wanted to get this working for my deployment of Synapse. The issue is with compatibility for older versions, I've no experience with twisted, async Python or the Synapse codebase so I was just following the instructions in this comment.

@motey
Copy link

motey commented Sep 30, 2020

The issue is with compatibility for older versions

ok, i see the issue. this would need some kind of version check to be productive.

but otherwise matrix-synapse-rest-password-provider should rather be compatible with a recent matrix version than breaking recent versions in favor of being combatible with older versions. the current state is the least desirable i think.

moreover current old server will still run. they will not suddenly download a more recent version of matrix-synapse-rest-password-provider just because there is a release on github.

anyway thanks a lot for the fix @devplayer0 ❤️

@anoadragon453
Copy link
Author

I can help review a PR if necessary.

@motey
Copy link

motey commented Mar 10, 2021

@ma1uta Is this repo still maintained?

If yes:
@devplayer0 Would be great if you can find some time for a PR

If you are too busy i would try to create a PR

@ma1uta
Copy link
Owner

ma1uta commented Mar 10, 2021

@ma1uta Is this repo still maintained?

Yes, but I not so good in Python.

@motey
Copy link

motey commented Mar 10, 2021

ok, no worries, i am a python coder :D although i am not very familiar with the matrix universe (in a coding sense)

I would love to contribute and help out wherever i can.
If @devplayer0 wont react i'll copy his code and make a PR and have a look into the whole rest-password-provider thing.

and i guess there are some more people dependent on this repo and therefore will help happily :)

@devplayer0
Copy link

devplayer0 commented Mar 10, 2021

Unfortunately I'm quite busy with university stuff at the moment :( I would like to clean my code up and maybe get it working with older versions of Synapse, but I don't see that being possible for a while.

EDIT: I suppose the handiest thing might be to make a PR with my changes and tag it as a breaking upgrade? @ma1uta Would that work?

@ma1uta
Copy link
Owner

ma1uta commented Mar 10, 2021

Yup, I can mark a new release with a new tag v0.2 for example if PR will break backward compatibility.

@devplayer0
Copy link

Great, I should actually be able to check through what I changed in my fork before and open a PR in a few hours.

@motey
Copy link

motey commented Jul 7, 2021

@ma1uta Bump :)

@ma1uta ma1uta closed this as completed in #8 Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants