Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make static final Set constants immutable #13087

Merged
merged 6 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lucene/CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ Improvements

* GITHUB#13092: `static final Map` constants have been made immutable (Dmitry Cherniachenko)

* GITHUB#13087: Some `static final Set` constants were mutable - rectified. (Dmitry Cherniachenko)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to 9.11

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add the new set impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


Optimizations
---------------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.lucene.analysis.miscellaneous;

import java.util.Collections;
import java.util.EnumSet;
import java.util.Set;
import org.apache.lucene.analysis.util.StemmerUtil;
Expand Down Expand Up @@ -51,7 +52,8 @@ public enum Foldings {

private final Set<Foldings> foldings;

public static final Set<Foldings> ALL_FOLDINGS = EnumSet.allOf(Foldings.class);
public static final Set<Foldings> ALL_FOLDINGS =
Collections.unmodifiableSet(EnumSet.allOf(Foldings.class));

static final char AA = '\u00C5'; // Å
static final char aa = '\u00E5'; // å
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
import java.io.Reader;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import org.apache.lucene.analysis.AbstractAnalysisFactory;
Expand Down Expand Up @@ -55,10 +53,8 @@ public class TestFactories extends BaseTokenStreamTestCase {

/** Factories that are excluded from testing it with random data */
private static final Set<Class<? extends AbstractAnalysisFactory>> EXCLUDE_FACTORIES_RANDOM_DATA =
new HashSet<>(
Arrays.asList(
DelimitedTermFrequencyTokenFilterFactory.class,
DelimitedBoostTokenFilterFactory.class));
Set.of(
DelimitedTermFrequencyTokenFilterFactory.class, DelimitedBoostTokenFilterFactory.class);

public void test() throws IOException {
for (String tokenizer : TokenizerFactory.availableTokenizers()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
import java.io.Reader;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import org.apache.lucene.analysis.AbstractAnalysisFactory;
Expand Down Expand Up @@ -53,10 +51,8 @@ public class TestFactories extends BaseTokenStreamTestCase {

/** Factories that are excluded from testing it with random data */
private static final Set<Class<? extends AbstractAnalysisFactory>> EXCLUDE_FACTORIES_RANDOM_DATA =
new HashSet<>(
Arrays.asList(
DelimitedTermFrequencyTokenFilterFactory.class,
DelimitedBoostTokenFilterFactory.class));
Set.of(
DelimitedTermFrequencyTokenFilterFactory.class, DelimitedBoostTokenFilterFactory.class);

public void test() throws IOException {
for (String tokenizer : TokenizerFactory.availableTokenizers()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.nio.file.Paths;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -80,7 +81,7 @@ public abstract class BackwardsCompatibilityTestBase extends LuceneTestCase {
// make sure we never miss a version.
assertTrue("Version: " + version + " missing", binaryVersions.remove(version));
}
BINARY_SUPPORTED_VERSIONS = binaryVersions;
BINARY_SUPPORTED_VERSIONS = Collections.unmodifiableSet(binaryVersions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We changed that code recently, maybe check also the other Ancient indexes test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestAncientIndicesCompatibility.UNSUPPORTED_INDEXES is already immutable:

Or did you mean something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok all fine.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@
import java.nio.file.StandardOpenOption;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.attribute.FileTime;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import org.apache.lucene.util.IOUtils;

/**
Expand Down Expand Up @@ -65,7 +64,7 @@ public final class NativeFSLockFactory extends FSLockFactory {
/** Singleton instance */
public static final NativeFSLockFactory INSTANCE = new NativeFSLockFactory();

private static final Set<String> LOCK_HELD = Collections.synchronizedSet(new HashSet<String>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are subtle differences between these two things (synchronized set and a concurrent set). I would leave this change to a separate issue so that it can be assessed separately (I think it's a good idea, but we need to track down all the uses and make sure nothing will break).

private static final Set<String> LOCK_HELD = ConcurrentHashMap.newKeySet();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think LOCK_HELP should be Set<Path>. This would make more sense as it would also be working with different slashes.

I know this isn't related to this cleanup task, but I just have seen it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will open a separate PR for this.


private NativeFSLockFactory() {}

Expand Down Expand Up @@ -127,7 +126,7 @@ protected Lock obtainFSLock(FSDirectory dir, String lockName) throws IOException
}
}

private static final void clearLockHeld(Path path) throws IOException {
private static void clearLockHeld(Path path) throws IOException {
boolean remove = LOCK_HELD.remove(path.toString());
if (remove == false) {
throw new AlreadyClosedException("Lock path was cleared but never marked as held: " + path);
Expand Down
10 changes: 4 additions & 6 deletions lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@
import java.lang.reflect.Method;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

/**
* A utility for keeping backwards compatibility on previously abstract methods (or similar
* replacements).
*
* <p>Before the replacement method can be made abstract, the old method must kept deprecated. If
* <p>Before the replacement method can be made abstract, the old method must be kept deprecated. If
* somebody still overrides the deprecated method in a non-final class, you must keep track, of this
* and maybe delegate to the old method in the subclass. The cost of reflection is minimized by the
* following usage of this class:
Expand Down Expand Up @@ -74,14 +73,13 @@
*/
public final class VirtualMethod<C> {

private static final Set<Method> singletonSet =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, perhaps (or merge with the issue above)?

Collections.synchronizedSet(new HashSet<Method>());
private static final Set<Method> singletonSet = ConcurrentHashMap.newKeySet();

private final Class<C> baseClass;
private final String method;
private final Class<?>[] parameters;
private final ClassValue<Integer> distanceOfClass =
new ClassValue<Integer>() {
new ClassValue<>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unrelated and does not work with Java 11 (when you subclass on "new" you had to be explicit in former times). I'd like to be explicit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted

@Override
protected Integer computeValue(Class<?> subclazz) {
return Integer.valueOf(reflectImplementationDistance(subclazz));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.apache.lucene.util;

import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.Set;
Expand All @@ -25,7 +26,8 @@

public class TestFilterIterator extends LuceneTestCase {

private static final Set<String> set = new TreeSet<>(Arrays.asList("a", "b", "c"));
private static final Set<String> set =
Collections.unmodifiableSet(new TreeSet<>(Arrays.asList("a", "b", "c")));

private static void assertNoMore(Iterator<?> it) {
assertFalse(it.hasNext());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@
import java.net.SocketException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;
import org.apache.lucene.store.DataInput;
import org.apache.lucene.store.DataOutput;
Expand All @@ -61,7 +61,7 @@
@SuppressForbidden(reason = "We need Unsafe to actually crush :-)")
public class TestSimpleServer extends LuceneTestCase {

static final Set<Thread> clientThreads = Collections.synchronizedSet(new HashSet<>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine even here as it only affects one test.

static final Set<Thread> clientThreads = ConcurrentHashMap.newKeySet();
static final AtomicBoolean stop = new AtomicBoolean();

/** Handles one client connection */
Expand Down
Loading