Implement dispose method for PoolingDataSource resource cleanup - #6515
Implement dispose method for PoolingDataSource resource cleanup#6515brianpursley wants to merge 1 commit into
Conversation
Sorry, something went wrong.
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/OpenAsync calls are released with a shutdown exception.
- Ensure ManualResetEventSlim used in sync-over-async path is disposed via using.
- 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.
Sorry, something went wrong.
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 PoolingDataSource dispose to unblock pending ReadAsync waiters 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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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 PoolingDataSource disposal so pending Open/OpenAsync waiters are released.
- Ensure ManualResetEventSlim used in the sync-over-async path is disposed via using.
- 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.
Sorry, something went wrong.
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 PoolingDataSource dispose to unblock pending Open/OpenAsync waits with a pool-shutdown exception.
- Ensure ManualResetEventSlim used in the sync-over-async path in RentAsync is 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.
Sorry, something went wrong.
|
|
||
| // 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:
Testing enhancements:
Fixes #6514