Implement dispose method for PoolingDataSource resource cleanup#6515
Implement dispose method for PoolingDataSource resource cleanup#6515brianpursley wants to merge 1 commit intonpgsql:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Improves PoolingDataSource disposal so pending connection opens waiting on an exhausted pool are released immediately (instead of waiting for the pool timeout), and adds coverage for this behavior (Fixes #6514).
Changes:
- Complete the idle-connector channel during disposal so blocked
Open/OpenAsynccalls are released with a shutdown exception. - Ensure
ManualResetEventSlimused in sync-over-async path is disposed viausing. - Add a pool test verifying that disposing a data source releases a blocked open attempt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Npgsql/PoolingDataSource.cs |
Completes the idle connector channel on dispose, disposes the pruning timer, and ensures a ManualResetEventSlim is disposed. |
test/Npgsql.Tests/PoolTests.cs |
Adds a regression test asserting that disposing an exhausted pool releases a blocked open. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Improves PoolingDataSource disposal behavior so pending connection open attempts don’t wait for the pool timeout after the pool is disposed, and adds a regression test for this scenario (Fixes #6514).
Changes:
- Complete the idle-connector channel during
PoolingDataSourcedispose to unblock pendingReadAsyncwaiters with a pool-shutdown error. - Dispose the pruning timer during pool dispose (sync and async paths).
- Add a test ensuring blocked opens from an exhausted pool are released promptly when the data source is disposed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Npgsql/PoolingDataSource.cs | Completes the idle-connector channel on dispose and tweaks sync wait handling; adds dispose overrides that also shut down the pruning timer. |
| test/Npgsql.Tests/PoolTests.cs | Adds a regression test validating that disposing a pooled data source releases blocked open attempts with the expected exception. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'll pause here and see if anyone has any thoughts on this PR before I do anything else. My goal is to unblock the waiters during dispose. But as I was doing that, I realized the pruning timer wasn't being disposed, so I thought I would go ahead and dispose it here too. It seems like Copilot has some concerns about disposing the timer though. I can do some more work in that area if needed, but didn't want to go too far off on a tangent. |
5081d5e to
acdfc12
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves PoolingDataSource shutdown behavior so that connection opens blocked on an exhausted pool are released promptly when the data source is disposed (instead of waiting for the pool timeout), and adds a regression test for that scenario.
Changes:
- Complete the idle-connector channel during
PoolingDataSourcedisposal so pendingOpen/OpenAsyncwaiters are released. - Ensure
ManualResetEventSlimused in the sync-over-async path is disposed viausing. - Add a pool-disposal regression test covering both sync and async disposal paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Npgsql/PoolingDataSource.cs | Completes idle channel on dispose; adds a disposed check in OpenNewConnector; disposes a wait handle in RentAsync. |
| test/Npgsql.Tests/PoolTests.cs | Adds a test ensuring disposal releases a blocked open from an exhausted pool with the expected exception. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
acdfc12 to
cbeaaeb
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves PoolingDataSource disposal behavior so that pending connection requests blocked on an exhausted pool are released promptly when the data source is disposed, and adds a regression test for that scenario (Fixes #6514).
Changes:
- Complete the idle-connector channel during
PoolingDataSourcedispose to unblock pendingOpen/OpenAsyncwaits with a pool-shutdown exception. - Ensure
ManualResetEventSlimused in the sync-over-async path inRentAsyncis properly disposed. - Add a new pool test covering both sync and async disposal releasing an exhausted-pool waiter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Npgsql/PoolingDataSource.cs |
Completes the idle-connector channel on dispose and ensures the sync wait handle is disposed. |
test/Npgsql.Tests/PoolTests.cs |
Adds a regression test verifying dispose releases a blocked open attempt with the expected exception. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Mark the idle connector channel as complete, which will release all waiting attempts on it with an exception, | ||
| // and prevent any further attempts to return connectors to it. | ||
| IdleConnectorWriter.TryComplete(); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override async ValueTask DisposeAsyncBase() | ||
| { | ||
| await base.DisposeAsyncBase().ConfigureAwait(false); | ||
|
|
||
| // Mark the idle connector channel as complete, which will release all waiting attempts on it with an exception, | ||
| // and prevent any further attempts to return connectors to it. | ||
| IdleConnectorWriter.TryComplete(); |
| // Make sure that the openTask is blocked | ||
| Assert.That(await Task.WhenAny(openTask, Task.Delay(100)), Is.Not.SameAs(openTask)); | ||
|
|
||
| // Dispose the pool while the openTask is waiting for a connection. This should cause the openTask to complete with an exception. | ||
| if (async) | ||
| await dataSource.DisposeAsync(); | ||
| else | ||
| dataSource.Dispose(); |
This pull request introduces improvements to the connection pool disposal logic in
PoolingDataSource, ensuring that pending connection requests are properly released when the pool is disposed. Additionally, a new test verifies this behavior. The most important changes are grouped below:Connection pool disposal improvements:
DisposeBaseandDisposeAsyncBaseoverrides inPoolingDataSourceto mark the idle connector channel as complete, ensuring all waiting connection attempts are released with an exception when the pool is disposed.ManualResetEventSliminRentAsyncto ensure proper disposal by using ausingstatement.Testing enhancements:
Dispose_releases_waiter_from_exhausted_pooltest inPoolTests.csto verify that disposing the pool releases blocked connection requests with an exception, covering both synchronous and asynchronous disposal scenarios.Fixes #6514