Skip to content

Commit 3ce15b5

Browse files
ejona86ejona86
committed
Reverse interceptor execution order
The previous order was unintuitive as the following would execute in the reverse order: Channel channel; channel = ClientInterceptors.intercept(channel, interceptor1, interceptor2); // vs channel = ClientInterceptors.intercept(channel, interceptor1); channel = ClientInterceptors.intercept(channel, interceptor2); After this change, they have equivalent behavior. With this change, there are no more per-invocation allocations and so calling 'next' twice is no longer prohibited. Resolves grpc#570
1 parent 6ca7eb3 commit 3ce15b5

4 files changed

Lines changed: 42 additions & 92 deletions

File tree

‎core/src/main/java/io/grpc/ClientInterceptors.java‎

Lines changed: 11 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,11 @@
3232
package io.grpc;
3333

3434
import com.google.common.base.Preconditions;
35-
import com.google.common.collect.ImmutableList;
3635

3736
import io.grpc.ForwardingClientCall.SimpleForwardingClientCall;
3837
import io.grpc.ForwardingClientCallListener.SimpleForwardingClientCallListener;
3938

4039
import java.util.Arrays;
41-
import java.util.Iterator;
4240
import java.util.List;
4341

4442
/**
@@ -51,7 +49,8 @@ private ClientInterceptors() {}
5149

5250
/**
5351
* Create a new {@link Channel} that will call {@code interceptors} before starting a call on the
54-
* given channel.
52+
* given channel. The last interceptor will have its {@link ClientInterceptor#interceptCall}
53+
* called first.
5554
*
5655
* @param channel the underlying channel to intercept.
5756
* @param interceptors array of interceptors to bind to {@code channel}.
@@ -63,59 +62,34 @@ public static Channel intercept(Channel channel, ClientInterceptor... intercepto
6362

6463
/**
6564
* Create a new {@link Channel} that will call {@code interceptors} before starting a call on the
66-
* given channel.
65+
* given channel. The last interceptor will have its {@link ClientInterceptor#interceptCall}
66+
* called first.
6767
*
6868
* @param channel the underlying channel to intercept.
6969
* @param interceptors a list of interceptors to bind to {@code channel}.
7070
* @return a new channel instance with the interceptors applied.
7171
*/
7272
public static Channel intercept(Channel channel, List<ClientInterceptor> interceptors) {
7373
Preconditions.checkNotNull(channel);
74-
if (interceptors.isEmpty()) {
75-
return channel;
74+
for (ClientInterceptor interceptor : interceptors) {
75+
channel = new InterceptorChannel(channel, interceptor);
7676
}
77-
return new InterceptorChannel(channel, interceptors);
77+
return channel;
7878
}
7979

8080
private static class InterceptorChannel extends Channel {
8181
private final Channel channel;
82-
private final Iterable<ClientInterceptor> interceptors;
82+
private final ClientInterceptor interceptor;
8383

84-
private InterceptorChannel(Channel channel, List<ClientInterceptor> interceptors) {
84+
private InterceptorChannel(Channel channel, ClientInterceptor interceptor) {
8585
this.channel = channel;
86-
this.interceptors = ImmutableList.copyOf(interceptors);
86+
this.interceptor = Preconditions.checkNotNull(interceptor, "interceptor");
8787
}
8888

8989
@Override
9090
public <ReqT, RespT> ClientCall<ReqT, RespT> newCall(
9191
MethodDescriptor<ReqT, RespT> method, CallOptions callOptions) {
92-
return new ProcessInterceptorChannel(channel, interceptors).newCall(method, callOptions);
93-
}
94-
}
95-
96-
private static class ProcessInterceptorChannel extends Channel {
97-
private final Channel channel;
98-
private Iterator<ClientInterceptor> interceptors;
99-
100-
private ProcessInterceptorChannel(Channel channel, Iterable<ClientInterceptor> interceptors) {
101-
this.channel = channel;
102-
this.interceptors = interceptors.iterator();
103-
}
104-
105-
@Override
106-
public <ReqT, RespT> ClientCall<ReqT, RespT> newCall(
107-
MethodDescriptor<ReqT, RespT> method, CallOptions callOptions) {
108-
if (interceptors != null && interceptors.hasNext()) {
109-
return interceptors.next().interceptCall(method, callOptions, this);
110-
} else {
111-
Preconditions.checkState(interceptors != null,
112-
"The channel has already been called. "
113-
+ "Some interceptor must have called on \"next\" twice.");
114-
interceptors = null;
115-
return channel.newCall(
116-
Preconditions.checkNotNull(method, "method"),
117-
Preconditions.checkNotNull(callOptions, "callOptions"));
118-
}
92+
return interceptor.interceptCall(method, callOptions, channel);
11993
}
12094
}
12195

‎core/src/main/java/io/grpc/ServerInterceptors.java‎

Lines changed: 16 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,11 @@
3232
package io.grpc;
3333

3434
import com.google.common.base.Preconditions;
35-
import com.google.common.collect.ImmutableList;
3635

3736
import io.grpc.ForwardingServerCall.SimpleForwardingServerCall;
3837
import io.grpc.ForwardingServerCallListener.SimpleForwardingServerCallListener;
3938

4039
import java.util.Arrays;
41-
import java.util.Iterator;
4240
import java.util.List;
4341

4442
/**
@@ -50,7 +48,8 @@ private ServerInterceptors() {}
5048

5149
/**
5250
* Create a new {@code ServerServiceDefinition} whose {@link ServerCallHandler}s will call
53-
* {@code interceptors} before calling the pre-existing {@code ServerCallHandler}.
51+
* {@code interceptors} before calling the pre-existing {@code ServerCallHandler}. The last
52+
* interceptor will have its {@link ServerInterceptor#interceptCall} called first.
5453
*
5554
* @param serviceDef the service definition for which to intercept all its methods.
5655
* @param interceptors array of interceptors to apply to the service.
@@ -63,7 +62,8 @@ public static ServerServiceDefinition intercept(ServerServiceDefinition serviceD
6362

6463
/**
6564
* Create a new {@code ServerServiceDefinition} whose {@link ServerCallHandler}s will call
66-
* {@code interceptors} before calling the pre-existing {@code ServerCallHandler}.
65+
* {@code interceptors} before calling the pre-existing {@code ServerCallHandler}. The last
66+
* interceptor will have its {@link ServerInterceptor#interceptCall} called first.
6767
*
6868
* @param serviceDef the service definition for which to intercept all its methods.
6969
* @param interceptors list of interceptors to apply to the service.
@@ -72,77 +72,46 @@ public static ServerServiceDefinition intercept(ServerServiceDefinition serviceD
7272
public static ServerServiceDefinition intercept(ServerServiceDefinition serviceDef,
7373
List<ServerInterceptor> interceptors) {
7474
Preconditions.checkNotNull(serviceDef);
75-
List<ServerInterceptor> immutableInterceptors = ImmutableList.copyOf(interceptors);
76-
if (immutableInterceptors.isEmpty()) {
75+
if (interceptors.isEmpty()) {
7776
return serviceDef;
7877
}
7978
ServerServiceDefinition.Builder serviceDefBuilder
8079
= ServerServiceDefinition.builder(serviceDef.getName());
8180
for (ServerMethodDefinition<?, ?> method : serviceDef.getMethods()) {
82-
wrapAndAddMethod(serviceDefBuilder, method, immutableInterceptors);
81+
wrapAndAddMethod(serviceDefBuilder, method, interceptors);
8382
}
8483
return serviceDefBuilder.build();
8584
}
8685

8786
private static <ReqT, RespT> void wrapAndAddMethod(
8887
ServerServiceDefinition.Builder serviceDefBuilder, ServerMethodDefinition<ReqT, RespT> method,
8988
List<ServerInterceptor> interceptors) {
90-
ServerCallHandler<ReqT, RespT> callHandler
91-
= InterceptCallHandler.create(interceptors, method.getServerCallHandler());
89+
ServerCallHandler<ReqT, RespT> callHandler = method.getServerCallHandler();
90+
for (ServerInterceptor interceptor : interceptors) {
91+
callHandler = InterceptCallHandler.create(interceptor, callHandler);
92+
}
9293
serviceDefBuilder.addMethod(method.withServerCallHandler(callHandler));
9394
}
9495

9596
private static class InterceptCallHandler<ReqT, RespT> implements ServerCallHandler<ReqT, RespT> {
9697
public static <ReqT, RespT> InterceptCallHandler<ReqT, RespT> create(
97-
List<ServerInterceptor> interceptors, ServerCallHandler<ReqT, RespT> callHandler) {
98-
return new InterceptCallHandler<ReqT, RespT>(interceptors, callHandler);
99-
}
100-
101-
private final List<ServerInterceptor> interceptors;
102-
private final ServerCallHandler<ReqT, RespT> callHandler;
103-
104-
private InterceptCallHandler(List<ServerInterceptor> interceptors,
105-
ServerCallHandler<ReqT, RespT> callHandler) {
106-
this.interceptors = interceptors;
107-
this.callHandler = callHandler;
108-
}
109-
110-
@Override
111-
public ServerCall.Listener<ReqT> startCall(String method, ServerCall<RespT> call,
112-
Metadata.Headers headers) {
113-
return ProcessInterceptorsCallHandler.create(interceptors.iterator(), callHandler)
114-
.startCall(method, call, headers);
115-
}
116-
}
117-
118-
private static class ProcessInterceptorsCallHandler<ReqT, RespT>
119-
implements ServerCallHandler<ReqT, RespT> {
120-
public static <ReqT, RespT> ProcessInterceptorsCallHandler<ReqT, RespT> create(
121-
Iterator<ServerInterceptor> interceptors, ServerCallHandler<ReqT, RespT> callHandler) {
122-
return new ProcessInterceptorsCallHandler<ReqT, RespT>(interceptors, callHandler);
98+
ServerInterceptor interceptor, ServerCallHandler<ReqT, RespT> callHandler) {
99+
return new InterceptCallHandler<ReqT, RespT>(interceptor, callHandler);
123100
}
124101

125-
private Iterator<ServerInterceptor> interceptors;
102+
private final ServerInterceptor interceptor;
126103
private final ServerCallHandler<ReqT, RespT> callHandler;
127104

128-
private ProcessInterceptorsCallHandler(Iterator<ServerInterceptor> interceptors,
105+
private InterceptCallHandler(ServerInterceptor interceptor,
129106
ServerCallHandler<ReqT, RespT> callHandler) {
130-
this.interceptors = interceptors;
107+
this.interceptor = Preconditions.checkNotNull(interceptor, "interceptor");
131108
this.callHandler = callHandler;
132109
}
133110

134111
@Override
135112
public ServerCall.Listener<ReqT> startCall(String method, ServerCall<RespT> call,
136113
Metadata.Headers headers) {
137-
if (interceptors != null && interceptors.hasNext()) {
138-
return interceptors.next().interceptCall(method, call, headers, this);
139-
} else {
140-
Preconditions.checkState(interceptors != null,
141-
"The call handler has already been called. "
142-
+ "Some interceptor must have called on \"next\" twice.");
143-
interceptors = null;
144-
return callHandler.startCall(method, call, headers);
145-
}
114+
return interceptor.interceptCall(method, call, headers, callHandler);
146115
}
147116
}
148117

‎core/src/test/java/io/grpc/ClientInterceptorsTest.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,20 +139,23 @@ public void channelAndInterceptorCalled() {
139139
verifyNoMoreInteractions(channel, interceptor);
140140
}
141141

142-
@Test(expected = IllegalStateException.class)
142+
@Test
143143
public void callNextTwice() {
144144
ClientInterceptor interceptor = new ClientInterceptor() {
145145
@Override
146146
public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
147147
MethodDescriptor<ReqT, RespT> method,
148148
CallOptions callOptions,
149149
Channel next) {
150-
next.newCall(method, callOptions);
150+
// Calling next twice is permitted, although should only rarely be useful.
151+
assertSame(call, next.newCall(method, callOptions));
151152
return next.newCall(method, callOptions);
152153
}
153154
};
154155
Channel intercepted = ClientInterceptors.intercept(channel, interceptor);
155-
intercepted.newCall(method, CallOptions.DEFAULT);
156+
assertSame(call, intercepted.newCall(method, CallOptions.DEFAULT));
157+
verify(channel, times(2)).newCall(same(method), same(CallOptions.DEFAULT));
158+
verifyNoMoreInteractions(channel);
156159
}
157160

158161
@Test
@@ -189,7 +192,7 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
189192
};
190193
Channel intercepted = ClientInterceptors.intercept(channel, interceptor1, interceptor2);
191194
assertSame(call, intercepted.newCall(method, CallOptions.DEFAULT));
192-
assertEquals(Arrays.asList("i1", "i2", "channel"), order);
195+
assertEquals(Arrays.asList("i2", "i1", "channel"), order);
193196
}
194197

195198
@Test

‎core/src/test/java/io/grpc/ServerInterceptorsTest.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,19 +158,23 @@ public void correctHandlerCalled() {
158158
verifyNoMoreInteractions(handler2);
159159
}
160160

161-
@Test(expected = IllegalStateException.class)
161+
@Test
162162
public void callNextTwice() {
163163
ServerInterceptor interceptor = new ServerInterceptor() {
164164
@Override
165165
public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(String method,
166166
ServerCall<RespT> call, Headers headers, ServerCallHandler<ReqT, RespT> next) {
167-
next.startCall(method, call, headers);
167+
// Calling next twice is permitted, although should only rarely be useful.
168+
assertSame(listener, next.startCall(method, call, headers));
168169
return next.startCall(method, call, headers);
169170
}
170171
};
171172
ServerServiceDefinition intercepted = ServerInterceptors.intercept(serviceDefinition,
172173
interceptor);
173-
getSoleMethod(intercepted).getServerCallHandler().startCall(methodName, call, headers);
174+
assertSame(listener,
175+
getSoleMethod(intercepted).getServerCallHandler().startCall(methodName, call, headers));
176+
verify(handler, times(2)).startCall(same(methodName), same(call), same(headers));
177+
verifyNoMoreInteractions(handler);
174178
}
175179

176180
@Test
@@ -207,7 +211,7 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(String method,
207211
serviceDefinition, Arrays.asList(interceptor1, interceptor2));
208212
assertSame(listener,
209213
getSoleMethod(intercepted).getServerCallHandler().startCall(methodName, call, headers));
210-
assertEquals(Arrays.asList("i1", "i2", "handler"), order);
214+
assertEquals(Arrays.asList("i2", "i1", "handler"), order);
211215
}
212216

213217
@Test

0 commit comments

Comments
 (0)