make use of Lock type - #6640
Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches several internal synchronization fields from object to System.Threading.Lock, motivated by reducing CPU overhead observed around monitor contention (notably in ResettableCancellationTokenSource.Start()).
Changes:
- Replaced several
objectlock fields withLockacross core and test code. - Updated some explicit monitor operations in
NpgsqlConnectorto useLock.Enter()/Exit()/TryEnter(). - Added the required
System.Threadingimport where needed.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Npgsql.Tests/Support/TestBase.cs | Uses Lock for the data source synchronization primitive in test infrastructure. |
| test/Npgsql.Tests/Support/SingleThreadSynchronizationContext.cs | Uses Lock for coordinating worker-thread startup/teardown in the custom sync context. |
| src/Npgsql/Util/ResettableCancellationTokenSource.cs | Uses Lock for guarding CTS lifecycle operations. |
| src/Npgsql/TypeMapping/GlobalTypeMapper.cs | Uses Lock for synchronizing global type-mapper mutable state. |
| src/Npgsql/Internal/NpgsqlConnector.cs | Converts key connector locks to Lock and replaces some Monitor.* calls with Lock APIs. |
Suppressed comments (2)
src/Npgsql/Internal/NpgsqlConnector.cs:2290
- Break() now uses
CleanupLock.Enter()/Exit(), but other paths (e.g. the already-broken fast path) still uselock (CleanupLock). Consider standardizing CleanupLock usage (preferably viaEnterScope()/Dispose) so it’s clear all call sites participate in the same synchronization mechanism.
CleanupLock.Enter();
src/Npgsql/Internal/NpgsqlConnector.cs:1957
- Same concern as the immediate cancellation path: this acquires CancelLock via
Enter(), while other parts of the file still uselock (CancelLock). Please consider switching all CancelLock usage to a single pattern (e.g.using (CancelLock.EnterScope()) { ... }) to avoid ambiguity and ensure consistent behavior.
CancelLock.Enter();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Monitor.Exit(SyncObj); | ||
| SyncObj.Exit(); | ||
| // Wait for the break to complete before going forward. | ||
| lock (CleanupLock) { } |
| // The connector is still alive, take the CancelLock before exiting SingleUseLock. | ||
| // If a break will happen after, it's going to wait for the cancellation to complete. | ||
| Monitor.Enter(CancelLock); | ||
| CancelLock.Enter(); |
|
Just FYI, don't expect any perf improvements from this. The only purpose of Lock type is to be explicit about its purpose, that's it. Under the hood it's still the same object and monitor. |
|
The referenced link mentions that |
We noticed some CPU time spent in
ResettableCancellationTokenSource.Start()->Monitor.Enter_Slowpath()in a CPU trace.I haven't actually measured whether/how much of a difference this change makes, but imho it makes sense either way since using the
Locktype is recommended over usingobject.