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.

Sign up for GitHub

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

Jump to bottom

Avoid dangling task-local connections after Database.disconnect() #211

Merged
merged 4 commits into from Sep 9, 2021
Merged

Avoid dangling task-local connections after Database.disconnect() #211

merged 4 commits into from Sep 9, 2021

Conversation

vmarkovtsev
Copy link
Contributor

This change forces initialization of a new Connection if we connect() again.

@vmarkovtsev
Avoid dangling task-local connections after Database.disconnect()
745714a 
This change forces initialization of a new Connection if we connect() again.
@tomchristie
Copy link
Member

Heya! Thanks for the PR here. I think I'd need a need clearer background here...
Even if it's too awkward to write a direct test case for this, it'd be good to walk through how we can demonstrate the issue?

@vmarkovtsev
Copy link
Contributor Author

Sure, here is what happens. Let's suppose we've got db = databases.Database(...).

  1. await db.connect().
  2. Run some query in some coroutine. db._connection_context sets to the task-local Connection in that coroutine context.
  3. await db.disconnect()
  4. We cannot await db.connect() anymore because db._connection_context is still pointing at the old Connection with a broken _backend, so the subsequent db queries are wrong.
  5. We continue referencing the connections for all the coroutine contexts that we touched.

The current code cleans up the global connection which we manipulate in case force_rollback is True. There is no simialr cleanup of the task-local connections.

@vmarkovtsev
Copy link
Contributor Author

@tomchristie Ping

@vmarkovtsev
Copy link
Contributor Author

@tomchristie ping

@vmarkovtsev
Copy link
Contributor Author

@tomchristie ping
(sorry for so many pings, I know how easy it is to miss a GitHub notification/PR review request, feel free to stop the pings 👍)

@vmarkovtsev
Copy link
Contributor Author

@tomchristie friendly ping

@vmarkovtsev vmarkovtsev mentioned this pull request Feb 2, 2021
Open
@taybin
Copy link
Contributor

taybin commented Sep 8, 2021

I've been seeing "Connection is not acquired" assertion errors in my long running processes. Could this be a cause of that?

@aminalaee
Copy link
Member

@taybin I'm not sure if this can be related to your issue.If you have more info about your issue, please open a new one.
Thanks for bringing this up anyway :)

@aminalaee aminalaee merged commit e3e7fa0 into encode:master Sep 9, 2021
@taybin
Copy link
Contributor

taybin commented Sep 9, 2021

@aminalaee On further research, I believe #283 is the cause of what I've been seeing.

@aminalaee
Copy link
Member

@taybin That one looks good too. I'll check it again.

aminalaee added a commit that referenced this pull request Sep 11, 2021
@aminalaee
swtiching to TEST_DATABASE_URL
339e106 
Avoid dangling task-local connections after Database.disconnect() (#211)

Co-authored-by: Amin Alaee <mohammadamin.alaee@gmail.com>

Reset counter for failed connections (#385)

Co-authored-by: Sergey Morozov <sergey@morozov.top>

Version 0.5.2 (#386)

update requirements

change scripts

update pytest config

update pytest

Update requirements

update workflow

try cache in workflow

disable coverage for py3.6

use cache in env

fix workflow

cache keys

update coverage workflow

test

test again

test

use absoulte path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aminalaee aminalaee aminalaee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vmarkovtsev @tomchristie @taybin @aminalaee