Avoid injecting service instance during

CoordinatedService.toString(). Add generic Provider<T> Json Moshi
adapter.

GitOrigin-RevId: 8e45f85d70ff9cdc3aa9d5f97adb4d25da056ef0

detekt.yaml

@@ -40,6 +40,8 @@ style:
  # Until moved to Guice 7, jakarta.inject.Provider causes issues with type overloads in Guice 6
  - value: 'jakarta.inject.Provider'
  reason: 'To be Guice 6 compliant, use `com.google.inject.Provider`'
+ excludes:
+ - '**/wisp/wisp-moshi/src/*/kotlin/wisp/moshi/ProviderJsonAdapter*.kt'
 
 detektive:
  active: true

misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt

@@ -17,8 +17,9 @@ import misk.web.dashboard.AdminDashboardAccess
 import misk.web.mediatype.MediaTypes
 import misk.web.metadata.toFormattedJson
 import misk.web.v2.DashboardPageLayout
+import wisp.moshi.ProviderJsonAdapterFactory
 import wisp.moshi.adapter
-import wisp.moshi.defaultKotlinMoshi
+import wisp.moshi.buildMoshi
 
 @Singleton
 class ServiceGraphTabIndexAction @Inject constructor(
@@ -37,7 +38,7 @@ class ServiceGraphTabIndexAction @Inject constructor(
  }
  }
  .build { _, _, _ ->
- val metadataArray = defaultKotlinMoshi
+ val metadataArray = buildMoshi(listOf(ProviderJsonAdapterFactory()))
  .adapter<List<ServiceGraphMetadata.GraphPairs>>()
  .toFormattedJson(serviceGraphMetadataProvider.get().graphVisual)
 

misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt

@@ -19,6 +19,8 @@ class AllMetadataActionTest {
 
  @Inject lateinit var action: AllMetadataAction
 
+ // TODO fix this test to rely on Misk injected moshi to generate response and assert against JSON instead of toString()
+ // so then can remove the manual fix in ServiceGraphBuilder.kt
  @Test
  fun `get all`() {
  val actual = action.getAll("all")

misk-config/src/main/kotlin/misk/web/metadata/Metadata.kt

@@ -2,7 +2,6 @@ package misk.web.metadata
 
 import com.squareup.moshi.JsonAdapter
 import kotlinx.html.TagConsumer
-import kotlinx.html.div
 import kotlinx.html.h3
 import misk.tailwind.components.AlertInfoHighlight
 import misk.tailwind.components.CodeBlock

misk-service/src/main/kotlin/misk/CoordinatedService.kt

@@ -1,23 +1,68 @@
 package misk
 
 import com.google.common.util.concurrent.AbstractService
+import com.google.common.util.concurrent.Atomics
 import com.google.common.util.concurrent.MoreExecutors
 import com.google.common.util.concurrent.Service
 import com.google.common.util.concurrent.Service.Listener
 import com.google.common.util.concurrent.Service.State
+import com.google.inject.Key
 import com.google.inject.Provider
+import com.google.inject.name.Named
+import misk.inject.toKey
 import java.util.concurrent.atomic.AtomicBoolean
 
 data class CoordinatedServiceMetadata(
  val dependencies: Set<String>,
  val directDependsOn: Set<String>,
 )
 
+internal inline fun <reified T : Service> CoordinatedService(serviceProvider: Provider<T>) = CoordinatedService(
+ key = T::class.toKey(),
+ serviceProvider = serviceProvider
+)
+
 internal class CoordinatedService(
+ private val key: Key<*>,
  private val serviceProvider: Provider<out Service>
 ) : AbstractService(), DelegatingService {
+
+ /**
+ * To avoid accessing the by lazy service property for toString, use key until the service
+ * is initiated by the service graph. This will be replaced with service.toString() during
+ * by lazy initialization.
+ */
+ private val toStringLazy = Atomics.newReference<() -> String> {
+ buildString {
+ key.annotationType?.let {
+ if (key.annotation is Named) {
+ append((key.annotation as Named).value)
+ } else {
+ append("@" + key.annotationType.simpleName + " ")
+ append(key.typeLiteral.rawType.simpleName)
+ }
+ } ?: append(key.typeLiteral.rawType.simpleName)
+
+ append(" [${this@CoordinatedService.state()}]")
+ if (this@CoordinatedService.state() == State.FAILED) {
+ append(" caused by: ${this@CoordinatedService.failureCause()}")
+ }
+ }
+ }
+
+ /**
+ * Use care when accessing this value as it will instantiate the instance of the class at this
+ * time. We want the instantiation to follow the Coordinated Service Graph.
+ */
  override val service: Service by lazy {
- serviceProvider.get()
+ val realService =
+ runCatching {
+ serviceProvider.get()
+ }
+ .onFailure {
+ this@CoordinatedService.notifyFailed(it)
+ }
+ .getOrThrow()
  .also { created ->
  created.checkNew("$created must be NEW for it to be coordinated")
  created.addListener(
@@ -42,6 +87,12 @@ internal class CoordinatedService(
  MoreExecutors.directExecutor()
  )
  }
+
+ toStringLazy.set {
+ realService.toString()
+ }
+
+ realService
  }
 
  /** Services this starts before aka enhancements or services who depend on this. */
@@ -57,15 +108,16 @@ internal class CoordinatedService(
  private val innerServiceStarted = AtomicBoolean(false)
 
  /**
- * Marks every [services] as a dependency and marks itself as directDependsOn on each service.
+ * Marks every [coordinatedServices] as a dependency and marks itself as
+ * directDependsOn on each service.
  * */
- fun addDependentServices(vararg services: CoordinatedService) {
+ fun addDependentServices(vararg coordinatedServices: CoordinatedService) {
  // Check that this service and all dependent services are new before modifying the graph.
  this.checkNew()
- for (service in services) {
- service.checkNew()
- dependencies += service
- service.directDependsOn += this
+ for (coordinatedService in coordinatedServices) {
+ coordinatedService.checkNew()
+ dependencies += coordinatedService
+ coordinatedService.directDependsOn += this
  }
  }
 
@@ -111,7 +163,11 @@ internal class CoordinatedService(
  }
  }
 
- override fun toString() = service.toString()
+ /**
+ * Get the service name from the current string source. This uses the key until the service
+ * is Injected to avoid early injecting the service out of band from the service graph.
+ */
+ override fun toString() = toStringLazy.get().invoke()
 
  /**
  * Traverses the dependency graph to detect cycles. If a cycle is found, then a
@@ -159,8 +215,10 @@ internal class CoordinatedService(
  }
  }
 
- fun toMetadata() = CoordinatedServiceMetadata(
- dependencies = dependencies.map { it.serviceProvider.get().javaClass.name }.toSet(),
- directDependsOn = directDependsOn.map { it.serviceProvider.get().javaClass.name }.toSet(),
- )
+ fun toMetadataProvider() = Provider {
+ CoordinatedServiceMetadata(
+ dependencies = dependencies.map { it.serviceProvider.get().javaClass.name }.toSet(),
+ directDependsOn = directDependsOn.map { it.serviceProvider.get().javaClass.name }.toSet(),
+ )
+ }
 }

misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt

@@ -8,12 +8,16 @@ import com.google.inject.Provider
 import misk.CoordinatedService.Companion.CycleValidity
 
 data class ServiceGraphBuilderMetadata(
- val serviceMap: Map<String, CoordinatedServiceMetadata>,
+ val serviceMap: Map<String, Provider<CoordinatedServiceMetadata>>,
  val serviceNames: Map<String, String>,
  /** A map of downstream services -> their upstreams. */
  val dependencyMap: Map<String, String>,
  val asciiVisual: String,
-)
+) {
+ override fun toString(): String {
+ return "ServiceGraphBuilderMetadata(serviceMap=${serviceMap.mapValues { (_,v) -> v.get() }}, serviceNames=$serviceNames, dependencyMap=$dependencyMap, asciiVisual=$asciiVisual)"
+ }
+}
 
 /**
  * Builds a graph of [CoordinatedService]s which defer start up and shut down until their dependent
@@ -40,7 +44,7 @@ internal class ServiceGraphBuilder {
  check(serviceMap[key] == null) {
  "Service $key cannot be registered more than once"
  }
- serviceMap[key] = CoordinatedService(serviceProvider)
+ serviceMap[key] = CoordinatedService(key, serviceProvider)
  serviceNames[key] = serviceName ?: "Anonymous Service(${key.typeLiteral.type.typeName})"
  }
 
@@ -155,7 +159,7 @@ internal class ServiceGraphBuilder {
  }
 
  fun toMetadata() = ServiceGraphBuilderMetadata(
- serviceMap = serviceMap.map { it.key.toString() to it.value.toMetadata() }.toMap(),
+ serviceMap = serviceMap.map { it.key.toString() to it.value.toMetadataProvider() }.toMap(),
  serviceNames = serviceNames.mapKeys { it.key.toString() },
  dependencyMap = dependencyMap.asMap().map { (k,v) -> k.toString() to v.toString() }.toMap(),
  asciiVisual = toString()

misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt

@@ -6,19 +6,21 @@ import misk.ServiceGraphBuilderMetadata
 import misk.web.metadata.Metadata
 import misk.web.metadata.MetadataProvider
 import misk.web.metadata.toFormattedJson
+import wisp.moshi.ProviderJsonAdapterFactory
 import wisp.moshi.adapter
-import wisp.moshi.defaultKotlinMoshi
+import wisp.moshi.buildMoshi
 
 data class ServiceGraphMetadata(
  val builderMetadata: ServiceGraphBuilderMetadata,
 ) : Metadata(
  metadata = builderMetadata,
- prettyPrint = "Service Graph Ascii Visual\n\n${builderMetadata.asciiVisual}\n\nMetadata\n\n" + defaultKotlinMoshi
+ prettyPrint = "Service Graph Ascii Visual\n\n${builderMetadata.asciiVisual}\n\nMetadata\n\n" + buildMoshi(listOf(ProviderJsonAdapterFactory()))
  .adapter<ServiceGraphBuilderMetadata>()
  .toFormattedJson(builderMetadata),
  descriptionString = "Guava service graph metadata, including a ASCII art visualization for easier debugging."
 ) {
- val graphVisual = generateGraphVisual()
+ /** Only evaluate this lazily to avoid initiating service startup through the [CoordinatedService::toMetadataProvider()] method. */
+ val graphVisual by lazy { generateGraphVisual() }
 
  data class GraphPairs(
  val source: String,
@@ -42,7 +44,7 @@ data class ServiceGraphMetadata(
  val output = mutableListOf<GraphPairs>()
 
  builderMetadata.serviceMap.forEach { (key, value) ->
- val dependencies = value.dependencies
+ val dependencies = value.get().dependencies
 
  dependencies.forEach { target ->
  output.add(GraphPairs(extractType(key), extractType(target)))

misk-service/src/test/kotlin/misk/CoordinatedServiceTest.kt

@@ -6,6 +6,7 @@ import com.google.inject.Key
 import com.google.inject.Provider
 import com.google.inject.name.Names
 import misk.ServiceGraphBuilderTest.AppendingService
+import misk.inject.toKey
 import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatThrownBy
 import org.junit.jupiter.api.RepeatedTest
@@ -23,7 +24,7 @@ class CoordinatedServiceTest {
  services.forEach { builder.addService(it) }
  val randomKeys = services.shuffled().map { it.first }.toMutableList()
  val dependencies = mutableListOf<Key<*>>()
- while(randomKeys.isNotEmpty()) {
+ while (randomKeys.isNotEmpty()) {
  val next = randomKeys.removeFirst()
  if (dependencies.isNotEmpty()) {
  builder.addDependency(dependent = dependencies.random(), dependsOn = next)
@@ -51,8 +52,8 @@ class CoordinatedServiceTest {
 
  val canStartService = service("canStartService")
  val neverStartedService = service("neverStartedService")
- val failOnStartService =
- key("failOnStartService") to FailOnStartService().toCoordinated()
+ val failOnStartService = (key("failOnStartService") to FailOnStartService())
+ .toCoordinated()
 
  val manager = ServiceGraphBuilder().also { builder ->
  builder.addService(canStartService)
@@ -83,7 +84,7 @@ class CoordinatedServiceTest {
 
  val service = service("service")
  val failOnStopService =
- key("failOnStopService") to FailOnStopService().toCoordinated()
+ (key("failOnStopService") to FailOnStopService()).toCoordinated()
 
  val manager = ServiceGraphBuilder().also { builder ->
  builder.addService(failOnStopService)
@@ -103,16 +104,13 @@ class CoordinatedServiceTest {
 
  @Test fun cannotAddRunningServiceAsDependency() {
  val target = StringBuilder()
- val runningService = CoordinatedService(
- Provider<Service> {
- AppendingService(target, "I will be running")
- }
- )
- val newService = CoordinatedService(
- Provider<Service> {
- AppendingService(target, "I will not run")
- }
- )
+ val runningService = CoordinatedService {
+ AppendingService(target, "I will be running")
+ }
+ val newService = CoordinatedService {
+ AppendingService(target, "I will not run")
+ }
+
 
  runningService.startAsync()
 
@@ -132,8 +130,9 @@ class CoordinatedServiceTest {
  service.startAsync()
 
  val failure = assertFailsWith<IllegalStateException> {
- CoordinatedService(Provider<Service> { service }).startAsync().awaitRunning()
+ CoordinatedService{ service }.service.startAsync().awaitRunning()
  }
+
  assertThat(failure).hasMessage("Running Service must be NEW for it to be coordinated")
 
  service.stopAsync()
@@ -148,13 +147,17 @@ class CoordinatedServiceTest {
  }
 
  private fun service(id: String): Pair<Key<*>, CoordinatedService> =
- key(id) to FakeService(id).toCoordinated()
+ (key(id) to FakeService(id)).toCoordinated()
 
  private fun ServiceGraphBuilder.addService(pair: Pair<Key<*>, Service>) {
  addService(pair.first, pair.second)
  }
 
- private fun Service.toCoordinated() = CoordinatedService(Provider { this })
+ private fun Pair<Key<*>, Service>.toCoordinated() =
+ this.first to CoordinatedService(
+ this.first,
+ Provider { this.second }
+ )
 
  private fun key(name: String) = Key.get(Service::class.java, Names.named(name))
 }