Skip to content

Commit 6669e53

Browse files
authored
Do not lock on reads of XPackLicenseState (#52492)
XPackLicenseState reads to necessary to validate a number of cluster operations. This reads occasionally occur on transport threads which should not be blocked. Currently we sychronize when reading. However, this is unecessary as only a single piece of state is updateable. This commit makes this state volatile and removes the locking.
1 parent c8ef964 commit 6669e53

File tree

1 file changed

+79
-61
lines changed

1 file changed

+79
-61
lines changed

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

Lines changed: 79 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import java.util.Objects;
2323
import java.util.concurrent.CopyOnWriteArrayList;
2424
import java.util.function.BiFunction;
25+
import java.util.function.Function;
26+
import java.util.function.Predicate;
2527

2628
/**
2729
* A holder for the current state of the license for all xpack features.
@@ -321,25 +323,41 @@ private static class Status {
321323
private final boolean isSecurityEnabled;
322324
private final boolean isSecurityExplicitlyEnabled;
323325

324-
private Status status = new Status(OperationMode.TRIAL, true);
326+
// Since Status is the only field that can be updated, we do not need to synchronize access to
327+
// XPackLicenseState. However, if status is read multiple times in a method, it can change in between
328+
// reads. Methods should use `executeAgainstStatus` and `checkAgainstStatus` to ensure that the status
329+
// is only read once.
330+
private volatile Status status = new Status(OperationMode.TRIAL, true);
325331

326332
public XPackLicenseState(Settings settings) {
327333
this.listeners = new CopyOnWriteArrayList<>();
328334
this.isSecurityEnabled = XPackSettings.SECURITY_ENABLED.get(settings);
329335
this.isSecurityExplicitlyEnabled = isSecurityEnabled && isSecurityExplicitlyEnabled(settings);
330336
}
331337

332-
private XPackLicenseState(XPackLicenseState xPackLicenseState) {
333-
this.listeners = xPackLicenseState.listeners;
334-
this.isSecurityEnabled = xPackLicenseState.isSecurityEnabled;
335-
this.isSecurityExplicitlyEnabled = xPackLicenseState.isSecurityExplicitlyEnabled;
336-
this.status = xPackLicenseState.status;
338+
private XPackLicenseState(List<LicenseStateListener> listeners, boolean isSecurityEnabled, boolean isSecurityExplicitlyEnabled,
339+
Status status) {
340+
341+
this.listeners = listeners;
342+
this.isSecurityEnabled = isSecurityEnabled;
343+
this.isSecurityExplicitlyEnabled = isSecurityExplicitlyEnabled;
344+
this.status = status;
337345
}
338346

339347
private static boolean isSecurityExplicitlyEnabled(Settings settings) {
340348
return settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey());
341349
}
342350

351+
/** Performs function against status, only reading the status once to avoid races */
352+
private <T> T executeAgainstStatus(Function<Status, T> statusFn) {
353+
return statusFn.apply(this.status);
354+
}
355+
356+
/** Performs predicate against status, only reading the status once to avoid races */
357+
private boolean checkAgainstStatus(Predicate<Status> statusPredicate) {
358+
return statusPredicate.test(this.status);
359+
}
360+
343361
/**
344362
* Updates the current state of the license, which will change what features are available.
345363
*
@@ -350,9 +368,7 @@ private static boolean isSecurityExplicitlyEnabled(Settings settings) {
350368
* trial was prior to this metadata being tracked (6.1)
351369
*/
352370
void update(OperationMode mode, boolean active, @Nullable Version mostRecentTrialVersion) {
353-
synchronized (this) {
354-
status = new Status(mode, active);
355-
}
371+
status = new Status(mode, active);
356372
listeners.forEach(LicenseStateListener::licenseStateChanged);
357373
}
358374

@@ -367,13 +383,13 @@ public void removeListener(final LicenseStateListener listener) {
367383
}
368384

369385
/** Return the current license type. */
370-
public synchronized OperationMode getOperationMode() {
371-
return status.mode;
386+
public OperationMode getOperationMode() {
387+
return executeAgainstStatus(status -> status.mode);
372388
}
373389

374390
/** Return true if the license is currently within its time boundaries, false otherwise. */
375-
public synchronized boolean isActive() {
376-
return status.active;
391+
public boolean isActive() {
392+
return checkAgainstStatus(status -> status.active);
377393
}
378394

379395
/**
@@ -435,26 +451,27 @@ public enum AllowedRealmType {
435451
/**
436452
* @return the type of realms that are enabled based on the license {@link OperationMode}
437453
*/
438-
public synchronized AllowedRealmType allowedRealmType() {
439-
final boolean isSecurityCurrentlyEnabled =
440-
isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled);
441-
if (isSecurityCurrentlyEnabled) {
442-
switch (status.mode) {
443-
case PLATINUM:
444-
case ENTERPRISE:
445-
case TRIAL:
446-
return AllowedRealmType.ALL;
447-
case GOLD:
448-
return AllowedRealmType.DEFAULT;
449-
case BASIC:
450-
case STANDARD:
451-
return AllowedRealmType.NATIVE;
452-
default:
453-
return AllowedRealmType.NONE;
454+
public AllowedRealmType allowedRealmType() {
455+
return executeAgainstStatus(status -> {
456+
final boolean isSecurityCurrentlyEnabled = isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled);
457+
if (isSecurityCurrentlyEnabled) {
458+
switch (status.mode) {
459+
case PLATINUM:
460+
case ENTERPRISE:
461+
case TRIAL:
462+
return AllowedRealmType.ALL;
463+
case GOLD:
464+
return AllowedRealmType.DEFAULT;
465+
case BASIC:
466+
case STANDARD:
467+
return AllowedRealmType.NATIVE;
468+
default:
469+
return AllowedRealmType.NONE;
470+
}
471+
} else {
472+
return AllowedRealmType.NONE;
454473
}
455-
} else {
456-
return AllowedRealmType.NONE;
457-
}
474+
});
458475
}
459476

460477
/**
@@ -674,8 +691,8 @@ public boolean isEnrichAllowed() {
674691
* <p>
675692
* EQL is available for all license types except {@link OperationMode#MISSING}
676693
*/
677-
public synchronized boolean isEqlAllowed() {
678-
return status.active;
694+
public boolean isEqlAllowed() {
695+
return checkAgainstStatus(status -> status.active);
679696
}
680697

681698
/**
@@ -736,17 +753,15 @@ public boolean isAnalyticsAllowed() {
736753
return isActive();
737754
}
738755

739-
public synchronized boolean isTrialLicense() {
740-
return status.mode == OperationMode.TRIAL;
741-
}
742-
743756
/**
744757
* @return true if security is available to be used with the current license type
745758
*/
746-
public synchronized boolean isSecurityAvailable() {
747-
OperationMode mode = status.mode;
748-
return mode == OperationMode.GOLD || mode == OperationMode.PLATINUM || mode == OperationMode.STANDARD ||
759+
public boolean isSecurityAvailable() {
760+
return checkAgainstStatus(status -> {
761+
OperationMode mode = status.mode;
762+
return mode == OperationMode.GOLD || mode == OperationMode.PLATINUM || mode == OperationMode.STANDARD ||
749763
mode == OperationMode.TRIAL || mode == OperationMode.BASIC || mode == OperationMode.ENTERPRISE;
764+
});
750765
}
751766

752767
/**
@@ -757,13 +772,15 @@ public synchronized boolean isSecurityAvailable() {
757772
* <li>xpack.security.enabled not specified as a setting</li>
758773
* </ul>
759774
*/
760-
public synchronized boolean isSecurityDisabledByLicenseDefaults() {
761-
switch (status.mode) {
762-
case TRIAL:
763-
case BASIC:
764-
return isSecurityEnabled && isSecurityExplicitlyEnabled == false;
765-
}
766-
return false;
775+
public boolean isSecurityDisabledByLicenseDefaults() {
776+
return checkAgainstStatus(status -> {
777+
switch (status.mode) {
778+
case TRIAL:
779+
case BASIC:
780+
return isSecurityEnabled && isSecurityExplicitlyEnabled == false;
781+
}
782+
return false;
783+
});
767784
}
768785

769786
public static boolean isTransportTlsRequired(License license, Settings settings) {
@@ -831,12 +848,12 @@ public static boolean isAllowedByOperationMode(
831848
* lived but instead used within a method when a consistent view of the license state
832849
* is needed for multiple interactions with the license state.
833850
*/
834-
public synchronized XPackLicenseState copyCurrentLicenseState() {
835-
return new XPackLicenseState(this);
851+
public XPackLicenseState copyCurrentLicenseState() {
852+
return executeAgainstStatus(status -> new XPackLicenseState(listeners, isSecurityEnabled, isSecurityExplicitlyEnabled, status));
836853
}
837854

838-
private synchronized boolean isAllowedBySecurity() {
839-
return isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled);
855+
private boolean isAllowedBySecurity() {
856+
return checkAgainstStatus(status -> isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled));
840857
}
841858

842859
/**
@@ -849,16 +866,17 @@ private synchronized boolean isAllowedBySecurity() {
849866
*
850867
* @return true if feature is allowed, otherwise false
851868
*/
852-
private synchronized boolean isAllowedByLicenseAndSecurity(
869+
private boolean isAllowedByLicenseAndSecurity(
853870
OperationMode minimumMode, boolean needSecurity, boolean needActive, boolean allowTrial) {
854-
855-
if (needSecurity && false == isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled)) {
856-
return false;
857-
}
858-
if (needActive && false == status.active) {
859-
return false;
860-
}
861-
return isAllowedByOperationMode(status.mode, minimumMode, allowTrial);
871+
return checkAgainstStatus(status -> {
872+
if (needSecurity && false == isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled)) {
873+
return false;
874+
}
875+
if (needActive && false == status.active) {
876+
return false;
877+
}
878+
return isAllowedByOperationMode(status.mode, minimumMode, allowTrial);
879+
});
862880
}
863881

864882
}

0 commit comments

Comments
 (0)