NightOwl888 commented on code in PR #1096:
URL: https://github.com/apache/lucenenet/pull/1096#discussion_r1919035360
##########
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:
The primary concern is about object lifetime. It is more important to ensure
that the list of disposable objects is allowed to go out of scope than it is to
ensure it is lazily created. The lifetime of `RandomizedContext` is from the
test discovery phase to the end of the test run and we need to ensure these
disposable instances go out of scope after the test or class is finished
executing. Calling `Clear()` after each test and/or class would be expensive,
so it is better to let the list go out of scope without clearing it by setting
the field to `null`.
I think that using a lock with a nullable `List<T>` field is simpler. These
aren't the only features that use the lock in the upstream code. It would allow
the other features of `RandomizedContext` to be added without having to go back
and rework it later to use a lock again.
In addition, there won't be any lock contention caused by the test framework
because NUnit is managing these events and they do not happen simultaneously.
Lock contention could only happen if the user explicitly calls the
`DisposeAfterTest()` and/or `DisposeAfterSuite()` methods in parallel. But for
users, the `TearDown()` and `OneTimeTearDown()` methods are generally better
options than these for calling `Dispose()` on disposable objects.
Thoughts?
--
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
