paulirwin commented on code in PR #1129: URL: https://github.com/apache/lucenenet/pull/1129#discussion_r1957371919
########## src/Lucene.Net.Tests/Support/TestConcurrentSet.cs: ########## @@ -0,0 +1,42 @@ +// Some tests adapted from Apache Harmony: +// https://github.com/apache/harmony/blob/02970cb7227a335edd2c8457ebdde0195a735733/classlib/modules/concurrent/src/test/java/ConcurrentHashMapTest.java +// https://github.com/apache/harmony/blob/02970cb7227a335edd2c8457ebdde0195a735733/classlib/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/CollectionsTest.java + +using Lucene.Net.Support; +using System.Collections.Generic; + +namespace Lucene.Net +{ + /* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + /// <summary> + /// Tests for <see cref="ConcurrentSet{T}"/>. + /// </summary> + /// <remarks> + /// See the <see cref="TestConcurrentSetBase"/> class for most of the test cases. + /// This class specializes the tests for the <see cref="ConcurrentSet{T}"/> class. + /// </remarks> + public class TestConcurrentSet : TestConcurrentSetBase + { + protected override ISet<T> NewSet<T>() + => new ConcurrentSet<T>(new HashSet<T>()); Review Comment: This is complete: 1. Removed the lazy-initialization logic of the `syncRoot` field by making it `private readonly object syncRoot = new object();`. This property was (correctly) accessed by every method in the type, so there's no need to lazy-initialize it, as it is expected that if you're creating the set, you're going to use it. The "cost" of initializing a new object once per set is lower than having to check if it has been initialized _twice_ in every set method. This should improve performance. 2. Fixed a thread-safety issue in GetEnumerator where the array was initialized based on the set count outside of the synchronization, so it is possible that the set could be modified between initializing the enumerator set and copying the array, possibly resulting in an error. 3. Added unit test for concurrently synchronizing on the public SyncRoot property while simultaneously calling the ConcurrentSet Add method. 4. Added unit test for ensuring GetEnumerator returns an enumerator that does not fail while the set is being modified. 5. Changed the default inner set created by the parameterless NewSet method to be LinkedHashSet. 6. Created two versions of TestSynchronizedSet, one that uses HashSet as the inner set and one that uses LinkedHashSet. I hope we do not need to go further for this PR. -- 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
