Skip to content

Commit 9311758

Browse files
lucaslopespicpaylucaslopespicpay
authored
fix: finalize span within the Promise chain to prevent race condition errors on delayed completion (#32)
* fix: finalize span within the Promise chain to prevent race condition errors on delayed completion * test: add test for HttpClientAspect
1 parent 1304c99 commit 9311758

File tree

2 files changed

+342
-1
lines changed

2 files changed

+342
-1
lines changed

‎src/Aspect/HttpClientAspect.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,6 @@ public function process(ProceedingJoinPoint $proceedingJoinPoint)
116116
$this->onFullFilled($span, $options),
117117
$this->onRejected($span, $options)
118118
);
119-
$span->finish();
120119

121120
return $result;
122121
}
@@ -157,6 +156,8 @@ private function onFullFilled(Span $span, array $options): callable
157156
$span->setTag('otel.status_code', 'OK');
158157

159158
$this->appendCustomResponseSpan($span, $options, $response);
159+
160+
$span->finish();
160161
};
161162
}
162163

@@ -174,6 +175,8 @@ private function onRejected(Span $span, array $options): callable
174175

175176
$this->appendCustomResponseSpan($span, $options, $exception->getResponse());
176177

178+
$span->finish();
179+
177180
return Create::rejectionFor($exception);
178181
};
179182
}

‎tests/HttpClientAspectTest.php‎

Lines changed: 338 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,338 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* This file is part of Hyperf + OpenCodeCo
6+
*
7+
* @link https://opencodeco.dev
8+
* @document https://hyperf.wiki
9+
* @contact leo@opencodeco.dev
10+
* @license https://github.com/opencodeco/hyperf-metric/blob/main/LICENSE
11+
*/
12+
13+
namespace HyperfTest;
14+
15+
use GuzzleHttp\Client;
16+
use GuzzleHttp\Promise\PromiseInterface;
17+
use GuzzleHttp\Psr7\Uri;
18+
use Hyperf\Di\Aop\ProceedingJoinPoint;
19+
use Hyperf\Tracer\Aspect\HttpClientAspect;
20+
use Hyperf\Tracer\SpanTagManager;
21+
use Hyperf\Tracer\SwitchManager;
22+
use OpenTracing\Span;
23+
use OpenTracing\Tracer;
24+
use PHPUnit\Framework\TestCase;
25+
use Psr\Http\Message\ResponseInterface;
26+
27+
/**
28+
* @internal
29+
* @coversNothing
30+
*/
31+
class HttpClientAspectTest extends TestCase
32+
{
33+
private Tracer $tracer;
34+
35+
private SwitchManager $switchManager;
36+
37+
private SpanTagManager $spanTagManager;
38+
39+
private HttpClientAspect $aspect;
40+
41+
private Span $mockSpan;
42+
43+
protected function setUp(): void
44+
{
45+
$this->tracer = $this->createMock(Tracer::class);
46+
$this->switchManager = $this->createMock(SwitchManager::class);
47+
$this->spanTagManager = $this->createMock(SpanTagManager::class);
48+
$this->mockSpan = $this->createMock(Span::class);
49+
50+
$this->tracer
51+
->expects($this->any())
52+
->method('startSpan')
53+
->willReturn($this->mockSpan);
54+
55+
$this->aspect = new HttpClientAspect($this->tracer, $this->switchManager, $this->spanTagManager);
56+
}
57+
58+
public function testProcessWithGuzzleSwitchDisabled(): void
59+
{
60+
$this->switchManager
61+
->expects($this->once())
62+
->method('isEnabled')
63+
->with('guzzle')
64+
->willReturn(false);
65+
66+
$proceedingJoinPoint = $this->createMock(ProceedingJoinPoint::class);
67+
68+
$proceedingJoinPoint
69+
->expects($this->once())
70+
->method('process')
71+
->willReturn('result');
72+
73+
$result = $this->aspect->process($proceedingJoinPoint);
74+
75+
$this->assertEquals('result', $result);
76+
}
77+
78+
public function testProcessWithNoAspectOption(): void
79+
{
80+
$this->switchManager
81+
->expects($this->once())
82+
->method('isEnabled')
83+
->with('guzzle')
84+
->willReturn(true);
85+
86+
$proceedingJoinPoint = $this->createMock(ProceedingJoinPoint::class);
87+
$proceedingJoinPoint->arguments = [
88+
'keys' => [
89+
'options' => ['no_aspect' => true],
90+
],
91+
];
92+
$proceedingJoinPoint
93+
->expects($this->once())
94+
->method('process')
95+
->willReturn('result');
96+
97+
$result = $this->aspect->process($proceedingJoinPoint);
98+
99+
$this->assertEquals('result', $result);
100+
}
101+
102+
public function testProcessSuccessfully(): void
103+
{
104+
$this->setupSuccessfulRequest();
105+
106+
$client = $this->createMock(Client::class);
107+
$client
108+
->expects($this->atLeastOnce())
109+
->method('getConfig')
110+
->willReturnCallback(function ($key) {
111+
if ($key === 'base_uri') {
112+
return new Uri('https://api.example.com');
113+
}
114+
if ($key === 'ignore_uri') {
115+
return false;
116+
}
117+
if ($key === 'uri_mask') {
118+
return [];
119+
}
120+
return null;
121+
});
122+
123+
$response = $this->createMock(ResponseInterface::class);
124+
$response
125+
->expects($this->once())
126+
->method('getStatusCode')
127+
->willReturn(200);
128+
129+
$this->mockSpan
130+
->expects($this->atLeastOnce())
131+
->method('setTag');
132+
133+
$this->mockSpan
134+
->expects($this->once())
135+
->method('finish');
136+
137+
$promise = $this->createMock(PromiseInterface::class);
138+
$promise
139+
->expects($this->once())
140+
->method('then')
141+
->willReturnCallback(function ($onFulfilled) use ($response, $promise) {
142+
$onFulfilled($response);
143+
return $promise;
144+
});
145+
146+
$proceedingJoinPoint = $this->createMock(ProceedingJoinPoint::class);
147+
$proceedingJoinPoint->className = 'TestClass';
148+
$proceedingJoinPoint->methodName = 'testMethod';
149+
$proceedingJoinPoint->arguments = [
150+
'keys' => [
151+
'method' => 'GET',
152+
'uri' => 'https://api.example.com/users',
153+
'options' => ['headers' => []],
154+
],
155+
];
156+
$proceedingJoinPoint
157+
->expects($this->once())
158+
->method('getInstance')
159+
->willReturn($client);
160+
$proceedingJoinPoint
161+
->expects($this->once())
162+
->method('process')
163+
->willReturn($promise);
164+
165+
$this->tracer
166+
->expects($this->once())
167+
->method('inject')
168+
->willReturnCallback(function ($context, $format, &$headers) {
169+
$headers['X-Trace-ID'] = 'test-trace-id';
170+
});
171+
172+
$result = $this->aspect->process($proceedingJoinPoint);
173+
174+
$this->assertInstanceOf(PromiseInterface::class, $result);
175+
$this->assertArrayHasKey('X-Trace-ID', $proceedingJoinPoint->arguments['keys']['options']['headers']);
176+
}
177+
178+
public function testProcessSuccessfullyWithoutBaseUri(): void
179+
{
180+
$this->setupSuccessfulRequest();
181+
182+
$client = $this->createMock(Client::class);
183+
$client
184+
->expects($this->atLeastOnce())
185+
->method('getConfig')
186+
->willReturnCallback(function ($key) {
187+
if ($key === 'base_uri') {
188+
return null;
189+
}
190+
if ($key === 'ignore_uri') {
191+
return false;
192+
}
193+
if ($key === 'uri_mask') {
194+
return [];
195+
}
196+
return null;
197+
});
198+
199+
$response = $this->createMock(ResponseInterface::class);
200+
$response
201+
->expects($this->once())
202+
->method('getStatusCode')
203+
->willReturn(200);
204+
205+
$this->mockSpan
206+
->expects($this->atLeastOnce())
207+
->method('setTag');
208+
209+
$this->mockSpan
210+
->expects($this->once())
211+
->method('finish');
212+
213+
$promise = $this->createMock(PromiseInterface::class);
214+
$promise
215+
->expects($this->once())
216+
->method('then')
217+
->willReturnCallback(function ($onFulfilled) use ($response, $promise) {
218+
$onFulfilled($response);
219+
return $promise;
220+
});
221+
222+
$proceedingJoinPoint = $this->createMock(ProceedingJoinPoint::class);
223+
$proceedingJoinPoint->className = 'TestClass';
224+
$proceedingJoinPoint->methodName = 'testMethod';
225+
$proceedingJoinPoint->arguments = [
226+
'keys' => [
227+
'method' => 'POST',
228+
'uri' => 'https://api.example.com/users',
229+
'options' => ['headers' => []],
230+
],
231+
];
232+
$proceedingJoinPoint
233+
->expects($this->once())
234+
->method('getInstance')
235+
->willReturn($client);
236+
$proceedingJoinPoint
237+
->expects($this->once())
238+
->method('process')
239+
->willReturn($promise);
240+
241+
$result = $this->aspect->process($proceedingJoinPoint);
242+
243+
$this->assertInstanceOf(PromiseInterface::class, $result);
244+
}
245+
246+
public function testProcessWithIgnoredUri(): void
247+
{
248+
$this->setupSuccessfulRequest();
249+
250+
$client = $this->createMock(Client::class);
251+
$client
252+
->expects($this->atLeastOnce())
253+
->method('getConfig')
254+
->willReturnCallback(function ($key) {
255+
if ($key === 'base_uri') {
256+
return new Uri('https://api.example.com');
257+
}
258+
if ($key === 'ignore_uri') {
259+
return true;
260+
}
261+
if ($key === 'uri_mask') {
262+
return [];
263+
}
264+
return null;
265+
});
266+
267+
$response = $this->createMock(ResponseInterface::class);
268+
$response
269+
->expects($this->once())
270+
->method('getStatusCode')
271+
->willReturn(200);
272+
273+
$this->mockSpan
274+
->expects($this->atLeastOnce())
275+
->method('setTag');
276+
277+
$this->mockSpan
278+
->expects($this->once())
279+
->method('finish');
280+
281+
$promise = $this->createMock(PromiseInterface::class);
282+
$promise
283+
->expects($this->once())
284+
->method('then')
285+
->willReturnCallback(function ($onFulfilled) use ($response, $promise) {
286+
$onFulfilled($response);
287+
return $promise;
288+
});
289+
290+
$proceedingJoinPoint = $this->createMock(ProceedingJoinPoint::class);
291+
$proceedingJoinPoint->className = 'TestClass';
292+
$proceedingJoinPoint->methodName = 'testMethod';
293+
$proceedingJoinPoint->arguments = [
294+
'keys' => [
295+
'method' => 'GET',
296+
'uri' => 'https://api.example.com/users/sensitive',
297+
'options' => ['headers' => []],
298+
],
299+
];
300+
$proceedingJoinPoint
301+
->expects($this->once())
302+
->method('getInstance')
303+
->willReturn($client);
304+
$proceedingJoinPoint
305+
->expects($this->once())
306+
->method('process')
307+
->willReturn($promise);
308+
309+
$result = $this->aspect->process($proceedingJoinPoint);
310+
311+
$this->assertInstanceOf(PromiseInterface::class, $result);
312+
}
313+
314+
private function setupSuccessfulRequest(): void
315+
{
316+
$this->switchManager
317+
->expects($this->once())
318+
->method('isEnabled')
319+
->with('guzzle')
320+
->willReturn(true);
321+
322+
$this->spanTagManager
323+
->expects($this->atLeastOnce())
324+
->method('has')
325+
->willReturn(true);
326+
327+
$this->spanTagManager
328+
->expects($this->atLeastOnce())
329+
->method('get')
330+
->willReturnCallback(function ($category, $key) {
331+
return "{$category}.{$key}";
332+
});
333+
334+
$this->tracer
335+
->expects($this->once())
336+
->method('inject');
337+
}
338+
}

0 commit comments

Comments
 (0)