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

Make MatrixClient asynchronous #145

Closed
wants to merge 3 commits into from

Conversation

non-Jedi
Copy link
Collaborator

@non-Jedi non-Jedi commented Sep 16, 2017

I haven't tested this yet, and I plan on splitting out the monster commit into
actual bite-size digestible commits, but I'm not entirely sure when I'll get
back working on this again, and there's a chance someone might find it useful.

Also, this is kind of a monstrous PR in scope, so I thought I should let
@Half-Shot know that it was incoming.

I'll sign off on this properly, too once I split out that single commit; don't
worry.

Closes #127

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

Reorg of these changes into a set of commits that actually makes
sense is still coming; don't worry.
@non-Jedi
Copy link
Collaborator Author

non-Jedi commented Sep 18, 2017

This PR is substantially ready. It's worked in all manual testing I've done, and
it doesn't break any automated tests. I just need to do the following things
before declaring it not a WIP.

  • Write some tests for new asynchronous api (not entirely sure how I'll go
    about this. Any help/advice appreciated)
  • Make this into a nice set of commits for review.

The following will be handled in a later PR.

  • Make login and registration apis asynchronous.
  • Make syncing multi-threaded rather than single-threaded.

Additionally, I have the following question that maybe @Half-Shot or, I dunno,
@erikjohnston (he does python, right? Please ping other members of core team who
may be better suited to answer questions for this sdk) might be able to help with:

  1. Should I write a series of locks for anything that is get/set from the
    matrix homeserver or should that be handled at a higher level? (e.g. if
    there's an ongoing request to set displayname, should that stop any
    requests to get displayname until it finishes?) EDIT: After talking to
    @richvdh:sw1v.org in #matrix-dev:matrix.org, I will not be implementing
    these sorts of locks--at least not yet.

@4nd3r
Copy link

4nd3r commented Sep 21, 2017

could this PR pave road to automatic (in SDK) connection retrying to HS after connection loss? or #123?

@non-Jedi non-Jedi changed the title Make MatrixClient asynchronous Make MatrixClient asynchronous (WIP) Sep 22, 2017
@non-Jedi non-Jedi changed the title Make MatrixClient asynchronous (WIP) Make MatrixClient asynchronous Sep 22, 2017
@non-Jedi
Copy link
Collaborator Author

non-Jedi commented Sep 22, 2017

@4nd3r this could definitely make that easier. #123 was a typo that I've now
fixed to #127. Most of the issues people have with retrying are with the
sync thread, which is actually already supposed to retry, but thanks to the
bug in #131, doesn't really.

But if what you're looking for is automatic retry for all requests, yes,
making the client async means we just have to implement that once within the
queue class.

@non-Jedi
Copy link
Collaborator Author

Note that this will make fixing #6 significantly easier.

@subins2000
Copy link

Any updates on this ?

@non-Jedi
Copy link
Collaborator Author

@subins2000 so kind of the state of the project right now:

I'm waiting on @Half-Shot (or anyone else familiar with the layout of the project) to review #160. After that, I'm planning on doing a 0.1.0 release, and then after the release, I'll be polishing this up (getting rid of merge conflicts), adding some tests, and then getting this reviewed to potentially merge.

@non-Jedi
Copy link
Collaborator Author

I don't think this is the right approach any more. Would prefer to build async on top of #168 once that's merged and make it exclusive to 3.5+.

@non-Jedi non-Jedi closed this May 14, 2018
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.

4 participants