Skip to content

Commit 20a944f

Browse files
loks0nclaude
loks0n
and
claude
committed
Fix: Eliminate shared-state races on the Http/Route/Router singletons
Under the Swoole and SwooleCoroutine adapters multiple requests run concurrently in the same process and shared a single Http instance plus the static Router/Route/Hook registries. Per-request data was being written onto those shared objects, so a coroutine yielding on I/O could return to find another request had clobbered its routing state. Four shared mutations are removed: 1. Http::$route — moved to the per-request container (coroutine-local under both Swoole adapters via Coroutine::getContext()), so getRoute() and the match() cache no longer collide between requests. Telemetry in run() now reads through getRoute(). 2. Route::$matchedPath — Router::match() now returns the matched route together with the prepared-path string instead of mutating the singleton Route. Http::match() stashes the path on the request container; execute() reads it via getMatchedPath(). 3. Http::$wildcardRoute — the wildcard branch now clones the singleton before stamping the request URI onto its $path, so two concurrent wildcard requests can't race on the shared route's path. 4. Hook::setParamValue() writeback in getArguments() — removed. The resolved $arguments array is still fed to the action; writing values back onto the shared Hook/Route was the only mutation that made shared hooks unsafe under concurrency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bddf77e commit 20a944f

3 files changed

Lines changed: 96 additions & 61 deletions

File tree

‎src/Http/Http.php‎

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,14 @@ class Http
106106
protected static array $requestHooks = [];
107107

108108
/**
109-
* Route
110-
*
111-
* Memory cached result for chosen route
109+
* Per-request container keys for the currently matched route and
110+
* the prepared-path string it was matched under. Storing these on
111+
* the per-request container (which is coroutine-local in the Swoole
112+
* adapters) keeps concurrent requests from clobbering each other's
113+
* routing state on the shared Http singleton.
112114
*/
113-
protected ?Route $route = null;
115+
private const string ROUTE_CONTEXT_KEY = '__utopia_http_route';
116+
private const string MATCHED_PATH_CONTEXT_KEY = '__utopia_http_matched_path';
114117

115118
/**
116119
* Wildcard route
@@ -465,19 +468,38 @@ public static function getRoutes(): array
465468
*/
466469
public function getRoute(): ?Route
467470
{
468-
return $this->route ?? null;
471+
$container = $this->server->getContainer();
472+
473+
return $container->has(self::ROUTE_CONTEXT_KEY) ? $container->get(self::ROUTE_CONTEXT_KEY) : null;
469474
}
470475

471476
/**
472477
* Set the current route
473478
*/
474-
public function setRoute(Route $route): self
479+
public function setRoute(?Route $route): self
475480
{
476-
$this->route = $route;
481+
$this->server->getContainer()->set(self::ROUTE_CONTEXT_KEY, fn() => $route);
477482

478483
return $this;
479484
}
480485

486+
/**
487+
* Read the prepared-path string the current request matched under.
488+
* Falls back to '' so callers (e.g. tests invoking execute() directly)
489+
* keep getting the legacy "first registered path-params" behavior.
490+
*/
491+
private function getMatchedPath(): string
492+
{
493+
$container = $this->server->getContainer();
494+
495+
return $container->has(self::MATCHED_PATH_CONTEXT_KEY) ? $container->get(self::MATCHED_PATH_CONTEXT_KEY) : '';
496+
}
497+
498+
private function setMatchedPath(string $path): void
499+
{
500+
$this->server->getContainer()->set(self::MATCHED_PATH_CONTEXT_KEY, fn() => $path);
501+
}
502+
481503
/**
482504
* Add Route
483505
*
@@ -590,18 +612,30 @@ public function start(): void
590612
*/
591613
public function match(Request $request, bool $fresh = true): ?Route
592614
{
593-
if (null !== $this->route && !$fresh) {
594-
return $this->route;
615+
if (!$fresh) {
616+
$cached = $this->getRoute();
617+
if (null !== $cached) {
618+
return $cached;
619+
}
595620
}
596621

597622
$url = parse_url($request->getURI(), PHP_URL_PATH);
598623
$url = \is_string($url) ? ($url === '' ? '/' : $url) : '/';
599624
$method = $request->getMethod();
600625
$method = (self::REQUEST_METHOD_HEAD === $method) ? self::REQUEST_METHOD_GET : $method;
601626

602-
$this->route = Router::match($method, $url);
627+
$matched = Router::match($method, $url);
628+
if (null === $matched) {
629+
$this->setRoute(null);
630+
$this->setMatchedPath('');
631+
return null;
632+
}
633+
634+
[$route, $matchedPath] = $matched;
635+
$this->setRoute($route);
636+
$this->setMatchedPath($matchedPath);
603637

604-
return $this->route;
638+
return $route;
605639
}
606640

607641
/**
@@ -612,7 +646,7 @@ public function execute(Route $route, Request $request, Response $response): sta
612646
$arguments = [];
613647
$groups = $route->getGroups();
614648

615-
$preparedPath = Router::preparePath($route->getMatchedPath());
649+
$preparedPath = Router::preparePath($this->getMatchedPath());
616650
$pathValues = $route->getPathValues($request, $preparedPath[0]);
617651

618652
try {
@@ -719,7 +753,6 @@ protected function getArguments(Hook $hook, array $values, array $requestParams)
719753
}
720754
}
721755

722-
$hook->setParamValue($key, $value);
723756
$arguments[$param['order']] = $value;
724757
}
725758

@@ -747,7 +780,7 @@ public function run(Request $request, Response $response): static
747780
$attributes = [
748781
'url.scheme' => $request->getProtocol(),
749782
'http.request.method' => $request->getMethod(),
750-
'http.route' => $this->route?->getPath(),
783+
'http.route' => $this->getRoute()?->getPath(),
751784
'http.response.status_code' => $response->getStatusCode(),
752785
];
753786
$this->requestDuration->record($requestDuration, $attributes);
@@ -854,12 +887,14 @@ private function runInternal(Request $request, Response $response): static
854887
}
855888

856889
if (null === $route && null !== self::$wildcardRoute) {
857-
$route = self::$wildcardRoute;
858-
$this->route = $route;
890+
// Clone so we can stamp the request URI onto $path without
891+
// mutating the singleton shared across concurrent coroutines.
892+
$route = clone self::$wildcardRoute;
859893
$path = parse_url($request->getURI(), PHP_URL_PATH);
860894
$path = \is_string($path) ? ($path === '' ? '/' : $path) : '/';
861895
$route->path($path);
862896

897+
$this->setRoute($route);
863898
$this->setRequestResource('route', fn() => $route, []);
864899
}
865900
if (null !== $route) {

‎src/Http/Router.php‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,14 @@ public static function addRouteAlias(string $path, Route $route): void
107107

108108
/**
109109
* Match route against the method and path.
110+
*
111+
* Returns the matched Route together with the prepared-path key it was
112+
* found under, so callers can resolve path params without mutating the
113+
* shared Route singleton.
114+
*
115+
* @return array{0: Route, 1: string}|null
110116
*/
111-
public static function match(string $method, string $path): ?Route
117+
public static function match(string $method, string $path): ?array
112118
{
113119
if (!\array_key_exists($method, self::$routes)) {
114120
return null;
@@ -129,9 +135,7 @@ public static function match(string $method, string $path): ?Route
129135
);
130136

131137
if (\array_key_exists($match, self::$routes[$method])) {
132-
$route = self::$routes[$method][$match];
133-
$route->setMatchedPath($match);
134-
return $route;
138+
return [self::$routes[$method][$match], $match];
135139
}
136140
}
137141

@@ -140,9 +144,7 @@ public static function match(string $method, string $path): ?Route
140144
*/
141145
$match = self::WILDCARD_TOKEN;
142146
if (\array_key_exists($match, self::$routes[$method])) {
143-
$route = self::$routes[$method][$match];
144-
$route->setMatchedPath($match);
145-
return $route;
147+
return [self::$routes[$method][$match], $match];
146148
}
147149

148150
/**
@@ -152,9 +154,7 @@ public static function match(string $method, string $path): ?Route
152154
$current = ($current ?? '') . "{$part}/";
153155
$match = $current . self::WILDCARD_TOKEN;
154156
if (\array_key_exists($match, self::$routes[$method])) {
155-
$route = self::$routes[$method][$match];
156-
$route->setMatchedPath($match);
157-
return $route;
157+
return [self::$routes[$method][$match], $match];
158158
}
159159
}
160160

‎tests/RouterTest.php‎

Lines changed: 35 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ public function testCanMatchUrl(): void
2323
Router::addRoute($routeAbout);
2424
Router::addRoute($routeAboutMe);
2525

26-
$this->assertEquals($routeIndex, Router::match(Http::REQUEST_METHOD_GET, '/'));
27-
$this->assertEquals($routeAbout, Router::match(Http::REQUEST_METHOD_GET, '/about'));
28-
$this->assertEquals($routeAboutMe, Router::match(Http::REQUEST_METHOD_GET, '/about/me'));
26+
$this->assertSame($routeIndex, Router::match(Http::REQUEST_METHOD_GET, '/')[0] ?? null);
27+
$this->assertSame($routeAbout, Router::match(Http::REQUEST_METHOD_GET, '/about')[0] ?? null);
28+
$this->assertSame($routeAboutMe, Router::match(Http::REQUEST_METHOD_GET, '/about/me')[0] ?? null);
2929
}
3030

3131
public function testCanMatchUrlWithPlaceholder(): void
@@ -44,12 +44,12 @@ public function testCanMatchUrlWithPlaceholder(): void
4444
Router::addRoute($routeBlogPostComments);
4545
Router::addRoute($routeBlogPostCommentsSingle);
4646

47-
$this->assertEquals($routeBlog, Router::match(Http::REQUEST_METHOD_GET, '/blog'));
48-
$this->assertEquals($routeBlogAuthors, Router::match(Http::REQUEST_METHOD_GET, '/blog/authors'));
49-
$this->assertEquals($routeBlogAuthorsComments, Router::match(Http::REQUEST_METHOD_GET, '/blog/authors/comments'));
50-
$this->assertEquals($routeBlogPost, Router::match(Http::REQUEST_METHOD_GET, '/blog/test'));
51-
$this->assertEquals($routeBlogPostComments, Router::match(Http::REQUEST_METHOD_GET, '/blog/test/comments'));
52-
$this->assertEquals($routeBlogPostCommentsSingle, Router::match(Http::REQUEST_METHOD_GET, '/blog/test/comments/:comment'));
47+
$this->assertSame($routeBlog, Router::match(Http::REQUEST_METHOD_GET, '/blog')[0] ?? null);
48+
$this->assertSame($routeBlogAuthors, Router::match(Http::REQUEST_METHOD_GET, '/blog/authors')[0] ?? null);
49+
$this->assertSame($routeBlogAuthorsComments, Router::match(Http::REQUEST_METHOD_GET, '/blog/authors/comments')[0] ?? null);
50+
$this->assertSame($routeBlogPost, Router::match(Http::REQUEST_METHOD_GET, '/blog/test')[0] ?? null);
51+
$this->assertSame($routeBlogPostComments, Router::match(Http::REQUEST_METHOD_GET, '/blog/test/comments')[0] ?? null);
52+
$this->assertSame($routeBlogPostCommentsSingle, Router::match(Http::REQUEST_METHOD_GET, '/blog/test/comments/:comment')[0] ?? null);
5353
}
5454

5555
public function testCanMatchUrlWithWildcard(): void
@@ -62,11 +62,11 @@ public function testCanMatchUrlWithWildcard(): void
6262
Router::addRoute($routeAbout);
6363
Router::addRoute($routeAboutWildcard);
6464

65-
$this->assertEquals($routeIndex, Router::match('GET', '/'));
66-
$this->assertEquals($routeAbout, Router::match('GET', '/about'));
67-
$this->assertEquals($routeAboutWildcard, Router::match('GET', '/about/me'));
68-
$this->assertEquals($routeAboutWildcard, Router::match('GET', '/about/you'));
69-
$this->assertEquals($routeAboutWildcard, Router::match('GET', '/about/me/myself/i'));
65+
$this->assertSame($routeIndex, Router::match('GET', '/')[0] ?? null);
66+
$this->assertSame($routeAbout, Router::match('GET', '/about')[0] ?? null);
67+
$this->assertSame($routeAboutWildcard, Router::match('GET', '/about/me')[0] ?? null);
68+
$this->assertSame($routeAboutWildcard, Router::match('GET', '/about/you')[0] ?? null);
69+
$this->assertSame($routeAboutWildcard, Router::match('GET', '/about/me/myself/i')[0] ?? null);
7070
}
7171

7272
public function testCanMatchHttpMethod(): void
@@ -77,11 +77,11 @@ public function testCanMatchHttpMethod(): void
7777
Router::addRoute($routeGET);
7878
Router::addRoute($routePOST);
7979

80-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/'));
81-
$this->assertEquals($routePOST, Router::match(Http::REQUEST_METHOD_POST, '/'));
80+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/')[0] ?? null);
81+
$this->assertSame($routePOST, Router::match(Http::REQUEST_METHOD_POST, '/')[0] ?? null);
8282

83-
$this->assertNotEquals($routeGET, Router::match(Http::REQUEST_METHOD_POST, '/'));
84-
$this->assertNotEquals($routePOST, Router::match(Http::REQUEST_METHOD_GET, '/'));
83+
$this->assertNotSame($routeGET, Router::match(Http::REQUEST_METHOD_POST, '/')[0]);
84+
$this->assertNotSame($routePOST, Router::match(Http::REQUEST_METHOD_GET, '/')[0]);
8585
}
8686

8787
public function testCanMatchAlias(): void
@@ -93,9 +93,9 @@ public function testCanMatchAlias(): void
9393

9494
Router::addRoute($routeGET);
9595

96-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/target'));
97-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias'));
98-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias2'));
96+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/target')[0] ?? null);
97+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias')[0] ?? null);
98+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias2')[0] ?? null);
9999
}
100100

101101
public function testCanMatchMultipleAliases(): void
@@ -108,10 +108,10 @@ public function testCanMatchMultipleAliases(): void
108108

109109
Router::addRoute($routeGET);
110110

111-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/target'));
112-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias1'));
113-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias2'));
114-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias3'));
111+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/target')[0] ?? null);
112+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias1')[0] ?? null);
113+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias2')[0] ?? null);
114+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/alias3')[0] ?? null);
115115
}
116116

117117
public function testCanMatchMix(): void
@@ -127,22 +127,22 @@ public function testCanMatchMix(): void
127127

128128
Router::addRoute($routeGET);
129129

130-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/'));
131-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/console'));
132-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/invite'));
133-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/login'));
134-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/recover'));
135-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/console/lorem/ipsum/dolor'));
136-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/auth/lorem/ipsum'));
137-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/register/lorem/ipsum'));
130+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/')[0] ?? null);
131+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/console')[0] ?? null);
132+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/invite')[0] ?? null);
133+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/login')[0] ?? null);
134+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/recover')[0] ?? null);
135+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/console/lorem/ipsum/dolor')[0] ?? null);
136+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/auth/lorem/ipsum')[0] ?? null);
137+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/register/lorem/ipsum')[0] ?? null);
138138
}
139139

140140
public function testCanMatchFilename(): void
141141
{
142142
$routeGET = new Route(Http::REQUEST_METHOD_GET, '/robots.txt');
143143

144144
Router::addRoute($routeGET);
145-
$this->assertEquals($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/robots.txt'));
145+
$this->assertSame($routeGET, Router::match(Http::REQUEST_METHOD_GET, '/robots.txt')[0] ?? null);
146146
}
147147

148148
public function testCannotFindUnknownRouteByPath(): void
@@ -156,7 +156,7 @@ public function testCannotFindUnknownRouteByMethod(): void
156156

157157
Router::addRoute($route);
158158

159-
$this->assertEquals($route, Router::match(Http::REQUEST_METHOD_GET, '/404'));
159+
$this->assertSame($route, Router::match(Http::REQUEST_METHOD_GET, '/404')[0] ?? null);
160160

161161
$this->assertNull(Router::match(Http::REQUEST_METHOD_POST, '/404'));
162162
}

0 commit comments

Comments
 (0)