From a995d9be1dfc5741713ccb265b2178bc2a760386 Mon Sep 17 00:00:00 2001
From: Aryeh Gregor <ayg@aryeh.name>
Date: Wed, 14 Aug 2019 14:46:47 +0300
Subject: [PATCH] Add recursion check to createService()

This will throw when trying to create a service while already in the
process of creating that same service, i.e., if there's a circular
service dependency. This would have saved me a whole bunch of debugging
time. :)

Change-Id: Id148d4f221f35f4069f3e0ab0069d13ca271df3d
---
 includes/libs/services/ServiceContainer.php   | 18 +++++++++++++
 .../libs/services/ServiceContainerTest.php    | 26 +++++++++++++++++++
 .../includes/libs/services/TestWiring1.php    |  0
 .../includes/libs/services/TestWiring2.php    |  0
 4 files changed, 44 insertions(+)
 rename tests/phpunit/{ => unit}/includes/libs/services/ServiceContainerTest.php (94%)
 rename tests/phpunit/{ => unit}/includes/libs/services/TestWiring1.php (100%)
 rename tests/phpunit/{ => unit}/includes/libs/services/TestWiring2.php (100%)

diff --git a/includes/libs/services/ServiceContainer.php b/includes/libs/services/ServiceContainer.php
index d1f10524d5..84755edb90 100644
--- a/includes/libs/services/ServiceContainer.php
+++ b/includes/libs/services/ServiceContainer.php
@@ -6,6 +6,7 @@ use InvalidArgumentException;
 use Psr\Container\ContainerInterface;
 use RuntimeException;
 use Wikimedia\Assert\Assert;
+use Wikimedia\ScopedCallback;
 
 /**
  * Generic service container.
@@ -77,6 +78,11 @@ class ServiceContainer implements ContainerInterface, DestructibleService {
 	 */
 	private $destroyed = false;
 
+	/**
+	 * @var array Set of services currently being created, to detect loops
+	 */
+	private $servicesBeingCreated = [];
+
 	/**
 	 * @param array $extraInstantiationParams Any additional parameters to be passed to the
 	 * instantiator function when creating a service. This is typically used to provide
@@ -433,10 +439,20 @@ class ServiceContainer implements ContainerInterface, DestructibleService {
 	 * @param string $name
 	 *
 	 * @throws InvalidArgumentException if $name is not a known service.
+	 * @throws RuntimeException if a circular dependency is detected.
 	 * @return object
 	 */
 	private function createService( $name ) {
 		if ( isset( $this->serviceInstantiators[$name] ) ) {
+			if ( isset( $this->servicesBeingCreated[$name] ) ) {
+				throw new RuntimeException( "Circular dependency when creating service! " .
+					implode( ' -> ', array_keys( $this->servicesBeingCreated ) ) . " -> $name" );
+			}
+			$this->servicesBeingCreated[$name] = true;
+			$removeFromStack = new ScopedCallback( function () use ( $name ) {
+				unset( $this->servicesBeingCreated[$name] );
+			} );
+
 			$service = ( $this->serviceInstantiators[$name] )(
 				$this,
 				...$this->extraInstantiationParams
@@ -458,6 +474,8 @@ class ServiceContainer implements ContainerInterface, DestructibleService {
 				}
 			}
 
+			$removeFromStack->consume();
+
 			// NOTE: when adding more wiring logic here, make sure importWiring() is kept in sync!
 		} else {
 			throw new NoSuchServiceException( $name );
diff --git a/tests/phpunit/includes/libs/services/ServiceContainerTest.php b/tests/phpunit/unit/includes/libs/services/ServiceContainerTest.php
similarity index 94%
rename from tests/phpunit/includes/libs/services/ServiceContainerTest.php
rename to tests/phpunit/unit/includes/libs/services/ServiceContainerTest.php
index 6e51883cfb..f9e820ac6b 100644
--- a/tests/phpunit/includes/libs/services/ServiceContainerTest.php
+++ b/tests/phpunit/unit/includes/libs/services/ServiceContainerTest.php
@@ -66,6 +66,32 @@ class ServiceContainerTest extends PHPUnit\Framework\TestCase {
 		$this->assertSame( 1, $count, 'instantiator should be called exactly once!' );
 	}
 
+	public function testGetServiceRecursionCheck() {
+		$services = $this->newServiceContainer();
+
+		$services->defineService( 'service1', function ( ServiceContainer $services ) {
+			$services->getService( 'service2' );
+		} );
+
+		$services->defineService( 'service2', function ( ServiceContainer $services ) {
+			$services->getService( 'service3' );
+		} );
+
+		$services->defineService( 'service3', function ( ServiceContainer $services ) {
+			$services->getService( 'service1' );
+		} );
+
+		$exceptionThrown = false;
+		try {
+			$services->getService( 'service1' );
+		} catch ( RuntimeException $e ) {
+			$exceptionThrown = true;
+			$this->assertSame( 'Circular dependency when creating service! ' .
+				'service1 -> service2 -> service3 -> service1', $e->getMessage() );
+		}
+		$this->assertTrue( $exceptionThrown, 'RuntimeException must be thrown' );
+	}
+
 	public function testGetService_fail_unknown() {
 		$services = $this->newServiceContainer();
 
diff --git a/tests/phpunit/includes/libs/services/TestWiring1.php b/tests/phpunit/unit/includes/libs/services/TestWiring1.php
similarity index 100%
rename from tests/phpunit/includes/libs/services/TestWiring1.php
rename to tests/phpunit/unit/includes/libs/services/TestWiring1.php
diff --git a/tests/phpunit/includes/libs/services/TestWiring2.php b/tests/phpunit/unit/includes/libs/services/TestWiring2.php
similarity index 100%
rename from tests/phpunit/includes/libs/services/TestWiring2.php
rename to tests/phpunit/unit/includes/libs/services/TestWiring2.php
-- 
2.20.1