Skip to content

Fix concurrent NpgsqlDataSource.Dispose and Bootstrap#6116

Merged
vonzshik merged 1 commit intomainfrom
6115-data-source-concurrent-dispose-bootstrap-fix
Aug 20, 2025
Merged

Fix concurrent NpgsqlDataSource.Dispose and Bootstrap#6116
vonzshik merged 1 commit intomainfrom
6115-data-source-concurrent-dispose-bootstrap-fix

Conversation

@vonzshik
Copy link
Contributor

@vonzshik vonzshik commented May 22, 2025

Fixes #6115

@vonzshik vonzshik requested a review from roji as a code owner May 22, 2025 12:17
@vonzshik
Copy link
Contributor Author

vonzshik commented May 22, 2025

Another possible fix is to use ManualResetEventSlim, which should simplify the code in Dispose, but probably will make the code in Bootstrap slightly more complicated, so probably isn't worth it.

xeromorph
xeromorph reviewed May 22, 2025
View reviewed changes
@ArtemSerostanov
Copy link
Contributor

ArtemSerostanov commented May 23, 2025

I presume a similar fix should be applied to ReplicationConnection._feedbackSemaphore as well?

@vonzshik
Copy link
Contributor Author

vonzshik commented May 23, 2025 •
edited
Loading

I presume a similar fix should be applied to ReplicationConnection._feedbackSemaphore as well?

Looking at the code, it seems like whenever you start a ReplicationConnection, it saves the returning enumerator, which DisposeAsync will enumerate and then dispose semaphore, so there shouldn't really be any concurrency.

@vonzshik vonzshik force-pushed the 6115-data-source-concurrent-dispose-bootstrap-fix branch from b625fa0 to 9929427 Compare June 20, 2025 14:48
@vonzshik
Copy link
Contributor Author

vonzshik commented Jun 20, 2025

We discussed this further, and according to dotnet/runtime#116735 (comment), it's not an issue to leave SemaphoreSlim to be disposed by the finalizer thread, especially since DataSource's lifetime is usually the same as for the app.

@vonzshik vonzshik requested a review from NinoFloris June 20, 2025 14:51
NinoFloris
NinoFloris approved these changes Aug 19, 2025
View reviewed changes
Copy link
Member

@NinoFloris NinoFloris left a comment

Choose a reason for hiding this comment

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

It's a bit of a hack but we can revisit all of this when (if) we redo our data source cleanup code.

@vonzshik vonzshik merged commit 6f8971c into main Aug 20, 2025
15 checks passed
@vonzshik vonzshik deleted the 6115-data-source-concurrent-dispose-bootstrap-fix branch August 20, 2025 09:45
vonzshik added a commit that referenced this pull request Aug 20, 2025
@vonzshik
Fix concurrent NpgsqlDataSource.Dispose and Bootstrap (#6116)
e2b6731 
Fixes #6115

(cherry picked from commit 6f8971c)
vonzshik added a commit that referenced this pull request Aug 20, 2025
@vonzshik
Fix concurrent NpgsqlDataSource.Dispose and Bootstrap (#6116)
2781000 
Fixes #6115

(cherry picked from commit 6f8971c)
@vonzshik
Copy link
Contributor Author

vonzshik commented Aug 20, 2025

Backported to 9.0.4 via e2b6731, 8.0.8 via 2781000

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

Reviewers

@NinoFloris NinoFloris NinoFloris approved these changes

@roji roji Awaiting requested review from roji roji is a code owner

+1 more reviewer

@xeromorph xeromorph xeromorph left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SemaphoreSlim instances should be disposed in a thread-safe manner

4 participants

@vonzshik @ArtemSerostanov @xeromorph @NinoFloris