Skip to content

Commit f227543

Browse files
cylx3126cylx3126
authored
Fix a bug in Spring Cloud Gateway if HttpClientFinalizer#send does not invoke, the span created at NettyRoutingFilterInterceptor can not stop. (#672)
1 parent 4adb343 commit f227543

File tree

7 files changed

+172
-10
lines changed

7 files changed

+172
-10
lines changed

‎CHANGES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ Release Notes.
1616
* Rename system env name from `sw_plugin_kafka_producer_config` to `SW_PLUGIN_KAFKA_PRODUCER_CONFIG`.
1717
* Support for ActiveMQ-Artemis messaging tracing.
1818
* Archive the expired plugins `impala-jdbc-2.6.x-plugin`.
19+
* Fix a bug in Spring Cloud Gateway if HttpClientFinalizer#send does not invoke, the span created at NettyRoutingFilterInterceptor can not stop.
1920

2021
#### Documentation
2122
* Update docs to describe `expired-plugins`.

‎apm-sniffer/optional-plugins/optional-spring-plugins/optional-spring-cloud/gateway-2.1.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/cloud/gateway/v21x/NettyRoutingFilterInterceptor.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ public static EnhancedInstance getInstance(Object o) {
7474
@Override
7575
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
7676
Object ret) throws Throwable {
77+
if (ContextManager.isActive()) {
78+
// if HttpClientFinalizerSendInterceptor does not invoke, we will stop the span there to avoid context leak.
79+
ContextManager.stopSpan();
80+
}
7781
return ret;
7882
}
7983

‎apm-sniffer/optional-plugins/optional-spring-plugins/optional-spring-cloud/gateway-2.1.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/cloud/gateway/v21x/NettyRoutingFilterInterceptorTest.java‎

Lines changed: 145 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,23 @@
2121
import java.security.Principal;
2222
import java.time.Instant;
2323
import java.util.HashMap;
24+
import java.util.List;
2425
import java.util.Map;
2526
import java.util.function.Function;
2627
import org.apache.skywalking.apm.agent.core.context.ContextManager;
28+
import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
29+
import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
30+
import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan;
31+
import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
32+
import org.apache.skywalking.apm.agent.core.context.trace.TraceSegment;
33+
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
34+
import org.apache.skywalking.apm.agent.test.helper.SegmentHelper;
2735
import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule;
2836
import org.apache.skywalking.apm.agent.test.tools.SegmentStorage;
2937
import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint;
38+
import org.apache.skywalking.apm.agent.test.tools.SpanAssert;
3039
import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner;
40+
import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
3141
import org.junit.Assert;
3242
import org.junit.Before;
3343
import org.junit.Rule;
@@ -47,6 +57,103 @@
4757

4858
@RunWith(TracingSegmentRunner.class)
4959
public class NettyRoutingFilterInterceptorTest {
60+
61+
private static class ServerWebExchangeEnhancedInstance implements ServerWebExchange, EnhancedInstance {
62+
private ContextSnapshot snapshot;
63+
Map<String, Object> attributes = new HashMap<>();
64+
65+
@Override
66+
public Object getSkyWalkingDynamicField() {
67+
return snapshot;
68+
}
69+
70+
@Override
71+
public void setSkyWalkingDynamicField(Object value) {
72+
this.snapshot = (ContextSnapshot) value;
73+
}
74+
75+
@Override
76+
public ServerHttpRequest getRequest() {
77+
return null;
78+
}
79+
80+
@Override
81+
public ServerHttpResponse getResponse() {
82+
return null;
83+
}
84+
85+
@Override
86+
public Map<String, Object> getAttributes() {
87+
return attributes;
88+
}
89+
90+
@Override
91+
public Mono<WebSession> getSession() {
92+
return null;
93+
}
94+
95+
@Override
96+
public <T extends Principal> Mono<T> getPrincipal() {
97+
return null;
98+
}
99+
100+
@Override
101+
public Mono<MultiValueMap<String, String>> getFormData() {
102+
return null;
103+
}
104+
105+
@Override
106+
public Mono<MultiValueMap<String, Part>> getMultipartData() {
107+
return null;
108+
}
109+
110+
@Override
111+
public LocaleContext getLocaleContext() {
112+
return null;
113+
}
114+
115+
@Override
116+
public ApplicationContext getApplicationContext() {
117+
return null;
118+
}
119+
120+
@Override
121+
public boolean isNotModified() {
122+
return false;
123+
}
124+
125+
@Override
126+
public boolean checkNotModified(Instant instant) {
127+
return false;
128+
}
129+
130+
@Override
131+
public boolean checkNotModified(String s) {
132+
return false;
133+
}
134+
135+
@Override
136+
public boolean checkNotModified(String s, Instant instant) {
137+
return false;
138+
}
139+
140+
@Override
141+
public String transformUrl(String s) {
142+
return null;
143+
}
144+
145+
@Override
146+
public void addUrlTransformer(Function<String, String> function) {
147+
148+
}
149+
150+
@Override
151+
public String getLogPrefix() {
152+
return null;
153+
}
154+
}
155+
156+
private final ServerWebExchangeEnhancedInstance enhancedInstance = new ServerWebExchangeEnhancedInstance();
50157
private final NettyRoutingFilterInterceptor interceptor = new NettyRoutingFilterInterceptor();
51158
@Rule
52159
public AgentServiceRule serviceRule = new AgentServiceRule();
@@ -151,12 +258,48 @@ public void testIsTraced() throws Throwable {
151258
interceptor.beforeMethod(null, null, new Object[]{exchange}, null, null);
152259
interceptor.afterMethod(null, null, null, null, null);
153260
Assert.assertEquals(exchange.getAttributes().get(NETTY_ROUTING_FILTER_TRACED_ATTR), true);
154-
Assert.assertNotNull(ContextManager.activeSpan());
261+
Assert.assertFalse(ContextManager.isActive());
155262

156-
ContextManager.stopSpan();
263+
// no more need this, span was stopped at interceptor#afterMethod
264+
// ContextManager.stopSpan();
157265

158266
interceptor.beforeMethod(null, null, new Object[]{exchange}, null, null);
159267
interceptor.afterMethod(null, null, null, null, null);
160268
Assert.assertEquals(exchange.getAttributes().get(NETTY_ROUTING_FILTER_TRACED_ATTR), true);
161269
}
270+
271+
@Test
272+
public void testWithNullDynamicField() throws Throwable {
273+
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
274+
interceptor.afterMethod(null, null, null, null, null);
275+
// no more need this, span was stopped at interceptor#afterMethod
276+
// ContextManager.stopSpan();
277+
final List<TraceSegment> traceSegments = segmentStorage.getTraceSegments();
278+
Assert.assertEquals(traceSegments.size(), 1);
279+
final List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegments.get(0));
280+
Assert.assertNotNull(spans);
281+
Assert.assertEquals(spans.size(), 1);
282+
SpanAssert.assertComponent(spans.get(0), ComponentsDefine.SPRING_CLOUD_GATEWAY);
283+
}
284+
285+
@Test
286+
public void testWithContextSnapshot() throws Throwable {
287+
final AbstractSpan entrySpan = ContextManager.createEntrySpan("/get", null);
288+
SpanLayer.asHttp(entrySpan);
289+
entrySpan.setComponent(ComponentsDefine.SPRING_WEBFLUX);
290+
enhancedInstance.setSkyWalkingDynamicField(ContextManager.capture());
291+
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
292+
interceptor.afterMethod(null, null, null, null, null);
293+
// no more need this, span was stopped at interceptor#afterMethod
294+
// ContextManager.stopSpan();
295+
ContextManager.stopSpan(entrySpan);
296+
final List<TraceSegment> traceSegments = segmentStorage.getTraceSegments();
297+
Assert.assertEquals(traceSegments.size(), 1);
298+
final List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegments.get(0));
299+
Assert.assertNotNull(spans);
300+
Assert.assertEquals(spans.size(), 2);
301+
SpanAssert.assertComponent(spans.get(0), ComponentsDefine.SPRING_CLOUD_GATEWAY);
302+
SpanAssert.assertComponent(spans.get(1), ComponentsDefine.SPRING_WEBFLUX);
303+
SpanAssert.assertLayer(spans.get(1), SpanLayer.HTTP);
304+
}
162305
}

‎apm-sniffer/optional-plugins/optional-spring-plugins/optional-spring-cloud/gateway-3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/cloud/gateway/v3x/NettyRoutingFilterInterceptor.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ private EnhancedInstance getInstance(Object o) {
8181
@Override
8282
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
8383
Object ret) throws Throwable {
84+
if (ContextManager.isActive()) {
85+
// if HttpClientFinalizerSendInterceptor does not invoke, we will stop the span there to avoid context leak.
86+
ContextManager.stopSpan();
87+
}
8488
return ret;
8589
}
8690

‎apm-sniffer/optional-plugins/optional-spring-plugins/optional-spring-cloud/gateway-3.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/cloud/gateway/v3x/NettyRoutingFilterInterceptorTest.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,8 @@ public void setUp() throws Exception {
170170
public void testWithNullDynamicField() throws Throwable {
171171
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
172172
interceptor.afterMethod(null, null, null, null, null);
173-
ContextManager.stopSpan();
173+
// no more need this, span was stopped at interceptor#afterMethod
174+
// ContextManager.stopSpan();
174175
final List<TraceSegment> traceSegments = segmentStorage.getTraceSegments();
175176
Assert.assertEquals(traceSegments.size(), 1);
176177
final List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegments.get(0));
@@ -187,7 +188,8 @@ public void testWithContextSnapshot() throws Throwable {
187188
enhancedInstance.setSkyWalkingDynamicField(ContextManager.capture());
188189
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
189190
interceptor.afterMethod(null, null, null, null, null);
190-
ContextManager.stopSpan();
191+
// no more need this, span was stopped at interceptor#afterMethod
192+
// ContextManager.stopSpan();
191193
ContextManager.stopSpan(entrySpan);
192194
final List<TraceSegment> traceSegments = segmentStorage.getTraceSegments();
193195
Assert.assertEquals(traceSegments.size(), 1);
@@ -207,9 +209,10 @@ public void testIsTraced() throws Throwable {
207209
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
208210
interceptor.afterMethod(null, null, null, null, null);
209211
Assert.assertEquals(enhancedInstance.getAttributes().get(NETTY_ROUTING_FILTER_TRACED_ATTR), true);
210-
Assert.assertNotNull(ContextManager.activeSpan());
212+
Assert.assertFalse(ContextManager.isActive());
211213

212-
ContextManager.stopSpan();
214+
// no more need this, span was stopped at interceptor#afterMethod
215+
// ContextManager.stopSpan();
213216

214217
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
215218
interceptor.afterMethod(null, null, null, null, null);

‎apm-sniffer/optional-plugins/optional-spring-plugins/optional-spring-cloud/gateway-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/cloud/gateway/v4x/NettyRoutingFilterInterceptor.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ private EnhancedInstance getInstance(Object o) {
8181
@Override
8282
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
8383
Object ret) throws Throwable {
84+
if (ContextManager.isActive()) {
85+
// if HttpClientFinalizerSendInterceptor does not invoke, we will stop the span there to avoid context leak.
86+
ContextManager.stopSpan();
87+
}
8488
return ret;
8589
}
8690

‎apm-sniffer/optional-plugins/optional-spring-plugins/optional-spring-cloud/gateway-4.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/spring/cloud/gateway/v4x/NettyRoutingFilterInterceptorTest.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,8 @@ public void setUp() throws Exception {
170170
public void testWithNullDynamicField() throws Throwable {
171171
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
172172
interceptor.afterMethod(null, null, null, null, null);
173-
ContextManager.stopSpan();
173+
// no more need this, span was stopped at interceptor#afterMethod
174+
// ContextManager.stopSpan();
174175
final List<TraceSegment> traceSegments = segmentStorage.getTraceSegments();
175176
Assert.assertEquals(traceSegments.size(), 1);
176177
final List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegments.get(0));
@@ -187,7 +188,8 @@ public void testWithContextSnapshot() throws Throwable {
187188
enhancedInstance.setSkyWalkingDynamicField(ContextManager.capture());
188189
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
189190
interceptor.afterMethod(null, null, null, null, null);
190-
ContextManager.stopSpan();
191+
// no more need this, span was stopped at interceptor#afterMethod
192+
// ContextManager.stopSpan();
191193
ContextManager.stopSpan(entrySpan);
192194
final List<TraceSegment> traceSegments = segmentStorage.getTraceSegments();
193195
Assert.assertEquals(traceSegments.size(), 1);
@@ -207,9 +209,10 @@ public void testIsTraced() throws Throwable {
207209
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
208210
interceptor.afterMethod(null, null, null, null, null);
209211
Assert.assertEquals(enhancedInstance.getAttributes().get(NETTY_ROUTING_FILTER_TRACED_ATTR), true);
210-
Assert.assertNotNull(ContextManager.activeSpan());
212+
Assert.assertFalse(ContextManager.isActive());
211213

212-
ContextManager.stopSpan();
214+
// no more need this, span was stopped at interceptor#afterMethod
215+
// ContextManager.stopSpan();
213216

214217
interceptor.beforeMethod(null, null, new Object[]{enhancedInstance}, null, null);
215218
interceptor.afterMethod(null, null, null, null, null);

0 commit comments

Comments
 (0)