Guava: Remove @Beta from Service and ServiceManager

[cpovirk]

• In Service, it looks like it might be nice to use Duration instead of (long, TimeUnit), but I'm not sure we're ready to take that plunge soon enough to worry about it -- and of course we want most people to run through ServiceManager (where we can add such overloads) anyway.

(Rumor has it that Jesse Wilson might be interested in this.)

[whodevil]

Any idea if this will happen? I've been using this API, and I'd love to see the Beta removed from it.

[swankjesse]

I am interested in this.

[netdpb]

It looks like java.time overloads have been added. Is this reasonable now, @cpovirk?

[cpovirk]

It's been on my list of items for the new issue-planning process. I've tied up the other few issues I was looking at, so this can be next.

[cpovirk]

Other open issues related to Service:

• Services that are stopped cannot be restarted #418
• AbstractService needs functionality to triggerShutdown or interrupt running Tasks. #1918
• RaceCondition with AbstractExecutionThreadService? #2966
• Feature Request: allow specifying dependencies in ServiceManager #1406
• Feature Request: add logging to ServiceManager.stopAsync #1405
• Add Service.State.isTerminal() #2403

Most of these look like features we could add after finalizing the existing API. The main exception is permitting restarting services, but that is a big enough change that we are unlikely to ever change the core Service classes for it, even when they are still @Beta APIs. (We may still provide some other way to emulate restartable services, just one that doesn't touch the core API.)

[cpovirk]

On the Duration front, we might wish that AbstractScheduledService.CustomScheduler were the name of the Duration-based API, rather than the long, TimeUnit-based (i.e., Schedule-based) API. But since that name is taken for the latter, we're likely to pick a clumsier name for the Duration-based method. But I'm not sure a rename of the existing API would be worth the trouble, especially since many Android users can't easily use Duration yet.

[cpovirk]

Also: I noticed recently that the Service and ServiceManager await* methods are void methods that throw TimeoutException. This seems contrary to JDK precedent, which appears to use TimeoutException exclusively for non-void methods. There are tradeoffs here (performance vs. chance of unintentionally ignoring a failure). It may also be worth noting that our await* methods can also throw IllegalStateException on failure, so we're already somewhat exception-oriented. Anyway, this feels more like an arguable wart than something we'd actually change.

[cpovirk]

Hmm, ServiceManager does have a no-Executor overload of its addListener method, which is contrary to our usual practices. We may want to take a look at removing that. But that shouldn't interfere with removing @Beta from the rest of the API.

[cpovirk]

ImmutableMultimap is an unfortunate return type for servicesByState(): We recommend either ImmutableListMultimap or ImmutableSetMultimap, as appropriate (here, ImmutableSetMultimap). Shockingly, this _might_ be the only place that we inappropriately returned ImmutableMultimap / Multimap. We could try to fix that if we wanted.

[cpovirk]

Note that we can improve the return type of servicesByState() compatibly by injecting a bridge method.

[swankjesse]

For #418, I think the best solution is to start a fresh instance of the Service class. That also makes it much easier to implement correct services.

For #1406 I’ve got a full implementation of this in Misk; here's an example test. The trickiest part of our solution was expressing dependency relationships without instantiating Service objects first. For example, we won’t construct SchemaMigratorService until DatabaseConnectionService is RUNNING. Our solution is deeply coupled to Guice because it’s what does the instance creation stuff. I found it very convenient to express service dependencies alongside DI.

[cpovirk]

Thanks. I suspect that we will keep the no-Executor overload of addListener as @Beta so that we can deprecate and remove it, but I'm optimistic that we can remove @Beta from the rest. I'll try to have an update by the end of the week.

[andrewparmet]

For #1406 we've done something kind of like what you have @swankjesse, but using a Kotlin DSL:

class CoordinatedServicesBuilder {
    private val dependencyGraph = GraphBuilder.directed().build<Service>()

    fun Service.dependsOn(vararg upstreamServices: Service) {
        dependencyGraph.addNode(this) // allow no dependencies
        upstreamServices.forEach { service ->
            dependencyGraph.putEdge(this, service)
        }
    }

    fun build(): Set<Service> {
        require(!Graphs.hasCycle(dependencyGraph))
        // Assemble the set of coordinated services
    }
}

fun createCoordinatedServices(init: CoordinatedServicesBuilder.() -> Unit) =
    CoordinatedServicesBuilder().apply(init).build()

Usage is then:

val serviceManager = 
    ServiceManager(
        createCoordinatedServices {
            serviceA.dependsOn(serviceB)
            serviceB.dependsOn(serviceC)
            serviceD.dependsOn()
        }
    )

We weren't worried about instantiation order. We also built an extension to ServiceManager to allow it to be used as a Service - since we have some Services that manage many other Services from one class. Not sure how much general utility that could find.

[cpovirk]

As hinted at in #2966, we aren't sure if we love the contract of AbstractExecutionService.triggerShutdown, so we might leave that @Beta, too, recommending that users who want similar functionality (and want to avoid @Beta APIs) attach a stopping listener to do that sort of work.

Nothing new has come up in the past couple days, though, so I've started work on deprecating the 1-arg addListener.

[edit: following after https://github.com/google/guava/commit/ce37aee980860798636f80ff67e0c00999a47696 ]

[cpovirk]

Progress:

• Deprecated 1-arg overload of ServiceManager.addListener.
• Made Duration overloads of awaitRunning and awaitTerminated final in all Abstract*Service classes.
• Changed return type of ServiceManager.servicesByState() to ImmutableSetMultimap (but also retained a method with the old signature for binary compatibility). [not yet mirrored out]

I'm planning to put @Beta on AbstractExecutionService.triggerShutdown and AbstractService.doCancelStart and then remove @Beta from the types under discussion.

If anyone objects, please speak up soon :)

[cpovirk]

Jesse / others: Let me know how this looks. (I tweaked Jesse's CL slightly to keep 2 individual methods @Beta, but other than that, it should look just how you'd expect.)

[swankjesse]

🎉

β

💯

Thanks!