Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Background await fix - #6736

Closed
selrahal wants to merge 3 commits into
matrix-org:developfrom
selrahal:background-await-fix
Closed

selrahal wants to merge 3 commits into
matrix-org:developfrom
selrahal:background-await-fix

Conversation

@selrahal

@selrahal selrahal commented Jan 17, 2020 •

Copy link
Copy Markdown

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off
  • Code style is correct (run the linters)

@selrahal
selrahal requested a review from babolivier January 17, 2020 22:02
Signed-off-by: Salem Elrahal <selrahal@localhost.localdomain>
@selrahal

selrahal commented Jan 17, 2020 •

Copy link
Copy Markdown
Author

I'm not tied to this changeset, but it worked as a stop-gap for me to perform my sqlite3 to postgres migration. Don't know much async python, feel free to just decline if this isn't helpful.

This is already being tracked in @babolivier issue: #6719

@babolivier

babolivier commented Jan 17, 2020 •

Copy link
Copy Markdown
Contributor

Hey @selrahal, and thanks for your contribution!

Unfortunately, this fix can't really work as do_next_background_update and has_completed_background_updates are asynchronous functions that should be awaited (either with a await if the code calling these functions has been converted to async/awayt, or with a yield otherwise). As you can see in the CI, your change doesn't really fix the issue.

On top of that, I've already opened a PR to fix that: #6718 ; the right solution is to port the script to async/await and as you can see it's not as trivial as it might seem (and that PR also fixes a bug on the error handling that explains why the CI on your PR is passing despite the script failing).

Because of these two reasons, I'm going to close this PR. Thanks for your contribution and effort nonetheless, and hopefully that won't prevent you from contributing further! 🙂

@babolivier babolivier closed this Jan 17, 2020
@babolivier
babolivier removed their request for review January 17, 2020 23:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants