fix microsoft#105562

jrieken · jrieken · commit f170b506b3d0 · 2020-09-23T10:58:43.000+02:00
diff --git a/src/vs/platform/instantiation/common/instantiationService.ts b/src/vs/platform/instantiation/common/instantiationService.ts
@@ -183,9 +183,15 @@ export class InstantiationService implements IInstantiationService {
 			}
 
 			for (const { data } of roots) {
-				// create instance and overwrite the service collections
-				const instance = this._createServiceInstanceWithOwner(data.id, data.desc.ctor, data.desc.staticArguments, data.desc.supportsDelayedInstantiation, data._trace);
-				this._setServiceInstance(data.id, instance);
+				// Repeat the check for this still being a service sync descriptor. That's because
+				// instantiating a dependency might have side-effect and recursively trigger instantiation
+				// so that some dependencies are now fullfilled already.
+				const instanceOrDesc = this._getServiceInstanceOrDescriptor(data.id);
+				if (instanceOrDesc instanceof SyncDescriptor) {
+					// create instance and overwrite the service collections
+					const instance = this._createServiceInstanceWithOwner(data.id, data.desc.ctor, data.desc.staticArguments, data.desc.supportsDelayedInstantiation, data._trace);
+					this._setServiceInstance(data.id, instance);
+				}
 				graph.removeNode(data);
 			}
 		}
diff --git a/src/vs/platform/instantiation/test/common/instantiationService.test.ts b/src/vs/platform/instantiation/test/common/instantiationService.test.ts
@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as assert from 'assert';
-import { createDecorator, optional, ServicesAccessor } from 'vs/platform/instantiation/common/instantiation';
+import { createDecorator, IInstantiationService, optional, ServicesAccessor } from 'vs/platform/instantiation/common/instantiation';
 import { InstantiationService } from 'vs/platform/instantiation/common/instantiationService';
 import { ServiceCollection } from 'vs/platform/instantiation/common/serviceCollection';
 import { SyncDescriptor } from 'vs/platform/instantiation/common/descriptors';
@@ -392,4 +392,36 @@ suite('Instantiation Service', () => {
 
 		assert.equal(serviceInstanceCount, 1);
 	});
+
+	test('Remote window / integration tests is broken #105562', function () {
+
+		const Service1 = createDecorator<any>('service1');
+		class Service1Impl {
+			constructor(@IInstantiationService insta: IInstantiationService) {
+				const c = insta.invokeFunction(accessor => accessor.get(Service2)); // THIS is the recursive call
+				assert.ok(c);
+			}
+		}
+		const Service2 = createDecorator<any>('service2');
+		class Service2Impl {
+			constructor() { }
+		}
+
+		// This service depends on Service1 and Service2 BUT creating Service1 creates Service2 (via recursive invocation)
+		// and then Servce2 should not be created a second time
+		const Service21 = createDecorator<any>('service21');
+		class Service21Impl {
+			constructor(@Service2 readonly service2: Service2Impl, @Service1 readonly service1: Service1Impl) { }
+		}
+
+		const insta = new InstantiationService(new ServiceCollection(
+			[Service1, new SyncDescriptor(Service1Impl)],
+			[Service2, new SyncDescriptor(Service2Impl)],
+			[Service21, new SyncDescriptor(Service21Impl)],
+		));
+
+		const obj = insta.invokeFunction(accessor => accessor.get(Service21));
+		assert.ok(obj);
+	});
+
 });

Original file line number	Diff line number	Diff line change
`@@ -183,9 +183,15 @@ export class InstantiationService implements IInstantiationService {`
`183`	`183`	`}`
`184`	`184`
`185`	`185`	`for (const { data } of roots) {`
`186`		`- // create instance and overwrite the service collections`
`187`		`- const instance = this._createServiceInstanceWithOwner(data.id, data.desc.ctor, data.desc.staticArguments, data.desc.supportsDelayedInstantiation, data._trace);`
`188`		`- this._setServiceInstance(data.id, instance);`
	`186`	`+ // Repeat the check for this still being a service sync descriptor. That's because`
	`187`	`+ // instantiating a dependency might have side-effect and recursively trigger instantiation`
	`188`	`+ // so that some dependencies are now fullfilled already.`
	`189`	`+ const instanceOrDesc = this._getServiceInstanceOrDescriptor(data.id);`
	`190`	`+ if (instanceOrDesc instanceof SyncDescriptor) {`
	`191`	`+ // create instance and overwrite the service collections`
	`192`	`+ const instance = this._createServiceInstanceWithOwner(data.id, data.desc.ctor, data.desc.staticArguments, data.desc.supportsDelayedInstantiation, data._trace);`
	`193`	`+ this._setServiceInstance(data.id, instance);`
	`194`	`+ }`
`189`	`195`	`graph.removeNode(data);`
`190`	`196`	`}`
`191`	`197`	`}`