Make PoolManager.ClearAllPools remove pools#4722
Conversation
Brar
force-pushed
the
RemovePoolsInClearAllPools
branch
2 times, most recently
from
1929eaf to
e61b9b4
Compare
|
We should have more narrator voices in our issues and PRs 🤣 |
|
@Brar just in case, did you intentionally close the pr? |
Huh, no Idea how that happened. |
src/Npgsql/PoolManager.cs
Outdated
| _pools = new(); | ||
| } | ||
| foreach (var pool in pools.Values) | ||
| pool.Dispose(); |
There was a problem hiding this comment.
There is a curious case with MultipleHosts. The way it's working right now is NpgsqlMultiHostDataSource has a list of MultiHostDataSourceWrapper which are actually referenced by connections. I think we shouldn't remove them from the pool here and instead do so in the NpgsqlMultiHostDataSource.Dispose.
Then there is also a question of calling Clear on MultiHostDataSourceWrapper...
There was a problem hiding this comment.
As it currently stands, only NpgsqlDataSource implements Dispose() and that one calls Clear() which is implemented by the derived classes.
MultiHostDataSourceWrapper.Clear() currently calls Clear() on the wrapped NpgsqlMultiHostDataSource which then calls Clear() on all the Per-Host-Pools.
I don't see a problem with disposing whatever DataSource we have in the PoolManager.
Am I missing something again?
Brar
force-pushed
the
RemovePoolsInClearAllPools
branch
from
1c0a934 to
0163c17
Compare
| } | ||
| // When using PoolManager (legacy code path setting NpgsqlConnection.ConnectionString), clearing a pool | ||
| // removes the pool (NpgsqlDataSource) from the PoolManager and disposes it. | ||
| // This leads to an ObjectDisposedException when calling NpgsqlDataSource.Get() here. |
There was a problem hiding this comment.
This is for making ReloadTypes work after Clear/ClearAll is called, when multiplexing is on, right (good catch)? If so maybe mention multiplexing in the comment to make this clearer.
src/Npgsql/MultiplexingDataSource.cs
Outdated
| // Force doing the startup checks again when disposed | ||
| // in order to reset the Connection's DataSource | ||
| if (disposing) | ||
| StartupCheckPerformed = false; |
There was a problem hiding this comment.
Can you provide a bit more context on why this is needed? AFAICT this flag is only consulted when opening the first connection on a multiplexing data source, I'm not sure why we'd need to reset it here.
There was a problem hiding this comment.
That's the simplest solution to make things work if you call ReloadTypes after opening the first multiplexing connection.
Without it, you fail in the next command's execution after calling it and from there it it very hard to recover the DataSource.
Brar
force-pushed
the
RemovePoolsInClearAllPools
branch
4 times, most recently
from
513c507 to
ac912da
Compare
Brar
force-pushed
the
RemovePoolsInClearAllPools
branch
from
ac912da to
e9ab0a8
Compare
|
/cc @Brar, data source disposal seems to have been properly implemented, so this can be revisited. |
Brar
force-pushed
the
RemovePoolsInClearAllPools
branch
from
e9ab0a8 to
b063465
Compare
3 participants
Closes #3387
If I read the code correctly it is as simple as that.Narrator: "It wasn't."I clearly must be missing something.Narrator: "Yes"