paulirwin commented on code in PR #1096:
URL: https://github.com/apache/lucenenet/pull/1096#discussion_r1918790740
##########
src/Lucene.Net.TestFramework/Support/Util/RandomizedContext.cs:
##########
@@ -189,19 +184,11 @@ internal void AddDisposableAtEnd(IDisposable resource)
internal void DisposeResources()
{
- if (toDisposeAtEnd is not null)
+ Lazy<ConcurrentQueue<IDisposable>>? toDispose =
Interlocked.Exchange(ref toDisposeAtEnd, null);
Review Comment:
This can be simplified and still be thread-safe if the Lazy is non-nullable
above. This line could be removed, and replace `toDispose` with
`toDisposeAtEnd` below this.
##########
src/Lucene.Net.TestFramework/Support/Util/RandomizedContext.cs:
##########
@@ -40,12 +40,7 @@ internal class RandomizedContext
private readonly long randomSeed;
private readonly string randomSeedAsHex;
private readonly long testSeed;
- private List<IDisposable>? toDisposeAtEnd = null;
-
- /// <summary>
- /// Coordination at context level.
- /// </summary>
- private readonly object contextLock = new object();
+ private Lazy<ConcurrentQueue<IDisposable>>? toDisposeAtEnd = null;
Review Comment:
I think this is now "double-lazy" - and the coalesce below removes the
thread-safety guarantees of `Lazy<T>`. I think this should be definitely
initialized to a `new Lazy<T>(() => ...)`, either with the default
thread-safety mode or an explicit one, rather than setting it to null. That way
when `.Value` is accessed, it will run the initializer callback if it hasn't
been created in a thread-safe way.
This will also simplify the code below, because the property
`ToDisposeAtEnd` can then just be `=> toDisposeAtEnd.Value;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
