Skip to content

Commit 96c8414

Browse files
committed
Automatically initialize AbstractEndpointSelector
Motivation: The `initialize()` method of `AbstractEndpointSelector` may access member fields of subclasses, `initialize()` couldn't be called in the constructor of `AbstractEndpointSelector`. Although the requirement is stated in the Javadoc, many users overlooked calling `initialize()` in their implementations. I propose adding a fallback logic that automatically initializes `AbstractEndpointSelector` if it hasn't been intialized by the time the first enpoint selection occurs, so users are protected from implmentation mistakes. Modifications: - Try to initialize `AbstractEndpointSelector` if it hasn't been initialized when `select(...)` method is called. Result: Make `AbstractEndpointSelector` auto-initialize to avoid user-side implementation errors.
1 parent 9194843 commit 96c8414

File tree

5 files changed

+34
-8
lines changed

5 files changed

+34
-8
lines changed

core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.Set;
2222
import java.util.concurrent.CompletableFuture;
2323
import java.util.concurrent.ScheduledExecutorService;
24+
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
2425

2526
import com.google.common.annotations.VisibleForTesting;
2627

@@ -38,8 +39,13 @@
3839
*/
3940
public abstract class AbstractEndpointSelector implements EndpointSelector {
4041

42+
private static final AtomicIntegerFieldUpdater<AbstractEndpointSelector> initializedUpdater =
43+
AtomicIntegerFieldUpdater.newUpdater(AbstractEndpointSelector.class, "initialized");
44+
4145
private final EndpointGroup endpointGroup;
4246
private final EndpointAsyncSelector asyncSelector;
47+
// 0 - not initialized, 1 - initialized
48+
private volatile int initialized;
4349

4450
/**
4551
* Creates a new instance that selects an {@link Endpoint} from the specified {@link EndpointGroup}.
@@ -59,15 +65,33 @@ protected final EndpointGroup group() {
5965
/**
6066
* Initialize this {@link EndpointSelector} to listen to the new endpoints emitted by the
6167
* {@link EndpointGroup}. The new endpoints will be passed to {@link #updateNewEndpoints(List)}.
68+
*
69+
* <p>This method is called automatically when the first selection is made. However, if you want to
70+
* start listening to the {@link EndpointGroup} earlier, you can call this method manually.
6271
*/
63-
@UnstableApi
6472
protected final void initialize() {
65-
endpointGroup.addListener(this::refreshEndpoints, true);
73+
tryInitialize();
74+
}
75+
76+
private void tryInitialize() {
77+
if (initialized == 1) {
78+
return;
79+
}
80+
81+
if (initializedUpdater.compareAndSet(this, 0, 1)) {
82+
endpointGroup.addListener(this::refreshEndpoints, true);
83+
}
84+
}
85+
86+
@VisibleForTesting
87+
boolean isInitialized() {
88+
return initialized == 1;
6689
}
6790

6891
@Override
6992
public CompletableFuture<Endpoint> select(ClientRequestContext ctx, ScheduledExecutorService executor,
7093
long timeoutMillis) {
94+
tryInitialize();
7195
return asyncSelector.select(ctx, executor, endpointGroup.selectionTimeoutMillis());
7296
}
7397

@@ -106,6 +130,7 @@ protected void onTimeout(ClientRequestContext ctx, long selectionTimeoutMillis)
106130
@Nullable
107131
@Override
108132
protected Endpoint selectNow(ClientRequestContext ctx) {
133+
tryInitialize();
109134
return AbstractEndpointSelector.this.selectNow(ctx);
110135
}
111136

core/src/main/java/com/linecorp/armeria/client/endpoint/DefaultEndpointSelector.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ final class DefaultEndpointSelector<T extends LoadBalancer<Endpoint, ClientReque
5252
}
5353
});
5454
}
55-
initialize();
5655
}
5756

5857
@Override

core/src/test/java/com/linecorp/armeria/client/ClientRequestContextDelayedInitTest.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ class TestEndpointSelector extends AbstractEndpointSelector {
8989

9090
protected TestEndpointSelector(EndpointGroup endpointGroup) {
9191
super(endpointGroup);
92-
initialize();
9392
}
9493

9594
@Nullable

core/src/test/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelectorTest.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,11 @@ void immediateSelection() {
4646
final Endpoint endpoint = Endpoint.of("foo");
4747
final ClientRequestContext ctx = ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/"));
4848
final AbstractEndpointSelector endpointSelector = newSelector(endpoint);
49+
assertThat(endpointSelector.isInitialized()).isFalse();
4950
assertThat(endpointSelector.select(ctx, ctx.eventLoop(), Long.MAX_VALUE))
5051
.isCompletedWithValue(endpoint);
52+
// The AbstractEndpointSelector should be initialized when the first selection is made.
53+
assertThat(endpointSelector.isInitialized()).isTrue();
5154
assertThat(endpointSelector.pendingFutures()).isEmpty();
5255
}
5356

@@ -56,8 +59,11 @@ void delayedSelection() {
5659
final DynamicEndpointGroup group = new DynamicEndpointGroup();
5760
final ClientRequestContext ctx = ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/"));
5861
final AbstractEndpointSelector endpointSelector = newSelector(group);
62+
assertThat(endpointSelector.isInitialized()).isFalse();
5963
final CompletableFuture<Endpoint> future = endpointSelector.select(ctx, ctx.eventLoop(),
6064
Long.MAX_VALUE);
65+
// The AbstractEndpointSelector should be initialized when the first selection is made.
66+
assertThat(endpointSelector.isInitialized()).isTrue();
6167
assertThat(future).isNotDone();
6268

6369
final Endpoint endpoint = Endpoint.of("foo");
@@ -128,7 +134,7 @@ void testRampingUpInitialSelection() {
128134
}
129135

130136
private static AbstractEndpointSelector newSelector(EndpointGroup endpointGroup) {
131-
final AbstractEndpointSelector selector = new AbstractEndpointSelector(endpointGroup) {
137+
return new AbstractEndpointSelector(endpointGroup) {
132138

133139
@Nullable
134140
@Override
@@ -137,7 +143,5 @@ public Endpoint selectNow(ClientRequestContext ctx) {
137143
return endpoints.isEmpty() ? null : endpoints.get(0);
138144
}
139145
};
140-
selector.initialize();
141-
return selector;
142146
}
143147
}

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,6 @@ private final class XdsEndpointGroupSelector extends AbstractEndpointSelector {
249249

250250
XdsEndpointGroupSelector(EndpointGroup endpointGroup) {
251251
super(endpointGroup);
252-
initialize();
253252
}
254253

255254
@Override

0 commit comments

Comments
 (0)