Skip to content

Fix deadlock while cancelling a query#5033

Merged
vonzshik merged 4 commits intomainfrom
5032-user-query-cancellation-deadlock-fix
Apr 23, 2023
Merged

Fix deadlock while cancelling a query#5033
vonzshik merged 4 commits intomainfrom
5032-user-query-cancellation-deadlock-fix

Conversation

@vonzshik
Copy link
Contributor

@vonzshik vonzshik commented Apr 13, 2023

Fixes #5032

While this pr should fix the issue, it might lead to a case where we won't cancel a running query if a cancellation token is triggered before we were able to read all of the responses for the prepended queries. The ideal fix (I think) would be to make cancellation fully async, but since we want to backport this to 7.0, 6.0 and 5.0...

@vonzshik vonzshik force-pushed the 5032-user-query-cancellation-deadlock-fix branch from 5c0b843 to 79f235d Compare April 14, 2023 11:43
@vonzshik vonzshik marked this pull request as ready for review April 14, 2023 12:18
@vonzshik vonzshik requested a review from roji as a code owner April 14, 2023 12:18
@baal2000
Copy link
Contributor

@vonzshik

...but since we want to backport this to 7.0, 6.0 and 5.0...

Is the same issue present in any of 6.0 releases or "backport" means something else?

@vonzshik
Copy link
Contributor Author

Is the same issue present in any of 6.0 releases or "backport" means something else?

Yeah, it was introduced with #4907, which was backported to 7.0.2, 6.0.9 and 5.0.16.

@baal2000
Copy link
Contributor

we won't cancel a running query if a cancellation token is triggered before we were able to read all of the responses for the prepended queries

Is there going to be a different fix in 8.0 where the cancellation is guaranteed to happen (async continuation, or some other way)?

@vonzshik
Copy link
Contributor Author

Is there going to be a different fix in 8.0 where the cancellation is guaranteed to happen (async continuation, or some other way)?

That's the plan, for now I'm waiting for @roji to take a look so we can discuss further.

@roji
Copy link
Member

roji commented Apr 20, 2023

This is high on my list, I'm finishing up some large/intensive unrelated work and will look at this soon.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a targeted fix for backporting - but yeah, if possible we should maybe think about it for 8.0...

@vonzshik vonzshik merged commit 276bb7d into main Apr 23, 2023
@vonzshik vonzshik deleted the 5032-user-query-cancellation-deadlock-fix branch April 23, 2023 19:50
vonzshik added a commit that referenced this pull request Apr 23, 2023
vonzshik added a commit that referenced this pull request Apr 23, 2023
vonzshik added a commit that referenced this pull request Apr 23, 2023
@vonzshik
Copy link
Contributor Author

Backported to 7.0.4 via 72bef2b, 6.0.10 via 106a012, 5.0.16 via 3881c6c.

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 this pull request may close these issues.

Npgsql 7.0.2 blocks threads NpgsqlConnector.PerformUserCancellation causing connection pool exhaustion

4 participants