Kibana: [Discuss]: Providing plugin's own start contract from core.getStartServices

[lukeelmers]

Summary

One recurring challenge we've seen with authoring new platform plugins is finding a way to access a plugin's _own_ start contract from within items that are registered in the setup method.

This is a particularly common issue for plugins which primarily expose services. One concrete example of this is the esaggs expression function in the data plugin: The function must be registered with the expressions plugin in the setup method, but the function itself relies on data.search, data.indexPatterns, and data.query.

The solution to this problem for accessing core or your own plugin's dependencies is to use core.getStartServices(), but you cannot get your own start contract from there.

A Sad Workaround 🙁

In data and visualizations this limitation was worked around by using kibana_utils/create_getter_setter to generate a getter/setter for each service. The setter was then called from start so the service could be available via the getter for items registered during setup:

// services.ts
import { createGetterSetter } from '../../kibana_utils/public';
import { DataPublicPluginStart } from './types';
export const [getQueryService, setQueryService] = createGetterSetter<
  DataPublicPluginStart['query']
>('Query');

// plugin.ts
import { setQueryService } from './services';

class DataPlugin {
  setup(core, { expressions }) {
    expressions.registerFunction(esaggs);
  }

  start() {
    const queryService = this.queryService.start();
    setQueryService(queryService);

    return {
      query: queryService,
      // ...
    };
  }
}

// esaggs.ts
import { getQueryService } from './services';

export const esaggs = () => ({
  name: 'esaggs',
  args: {...},
  // ...,
  async fn(input, args) {
    const { filterManager } = getQueryService();
    // ...
    return table;
  },
});

This approach is problematic for a number of reasons, including:

1. Increased complexity of test mocks. Downstream plugins don't know or care whether data is using setters to manage dependencies internally, however mocked values now need to be added to each of the setters to ensure tests don't break.
2. Poor error handling. We've seen several incidents of bugs where users see a toast when retrieving a value from a getter fails, e.g. "UiSettings" is not set.
3. Potential for regressions in legacy plugins. The cause of most of the ambiguous errors is needing to have the setters re-set in the legacy world, which doesn't share an instance of these modules with the new platform. That means we've added some ugly code to ui/new_platform to ensure services get set another time, which is about to get worse as the visualizations plugin is migrated.

As a result, we are moving away from createGetterSetter for dealing with core and plugins dependencies (in favor of getStartServices), however the problem still exists for accessing internal start contracts.

I have also explored a solution which basically re-implements our own version of getStartServices (@stacey-gammon has done this as well). This is IMO better than the getters/setters, but still added boilerplate for each of our plugins, which could be avoided by...

My Proposal 🙂

I'd like to discuss returning a plugin's own start contract from core.getStartServices():

const [coreStart, depsStart, selfStart] = await core.getStartServices();

Since startDependencies$.next is already called after the plugin's startContract is retrieved in PluginWrapper, it seems the value could be easily passed along there:

const startContract = await this.instance.start(startContext, plugins);

this.startDependencies$.next([startContext, plugins, startContract]);

return startContract;

Benefits

1. Unified solution. Instead of leaving it to plugins to develop their own solutions, we can provide one cohesive answer to this question.
2. Prevents more tech debt. If we do this soon, we can prevent plugins from going further down the road of creating suboptimal workarounds, which have already created unnecessary tech debt and instability.
3. Nothing new for plugins to do. This is already exposed by core on the client & server, so folks wouldn't need to pick up any new tools, just retrieve the new value from getStartServices.

Drawbacks

1. More complex CoreSetup generic. As @pgayvallet pointed out, this would require CoreSetup to have another parameter as it would need to be aware of a plugin's own startContract:

interface CoreSetup<TPluginsStart extends object = object, TSelfStart extends object = object> {
  // ...
  getStartServices(): Promise<[CoreStart, TPluginsStart, TSelfStart]>;
}

Alternatives

Another alternative if we really don't want to touch getStartServices would be providing a recommended pattern for dealing with this use case. Perhaps with a helper which could replace the existing createGetterSetter in kibana_utils.

Questions

1. Are there any considerations for how this would affect the getStartServices provided by LegacyPlatformService? I assume this feature would simply not be available in legacy.
2. Any testing considerations to worry about here? createCoreSetupMock may need to be modified so that you could pass mock return values to getStartServices:

function createCoreSetupMock({
  basePath = '',
  pluginStartMocks = {},
  selfStartMocks = {},
} = {}) {
  const mock = {
    ...
    getStartServices: jest.fn<Promise<[ReturnType<typeof createCoreStartMock>, object, object ]>, []>(() =>
      Promise.resolve([createCoreStartMock({ basePath }), pluginStartMocks, selfStartMocks])
    ),
  };

  return mock;
}

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[lizozom]

One more thing to consider - createGetterSetter proves very effective when you have complex ui components like search bar. If we want to propagate all the required services from the top level, we would have to add them to all of our props.

[pgayvallet]

I understand the need and I personally don't have any objection on implementing that. Let's see what the others have to say about it.

Are there any considerations for how this would affect the getStartServices provided by LegacyPlatformService? I assume this feature would simply not be available in legacy.

LegacyCoreSetup got it's own type. We should be able to declare it's own getStartServices signature to exclude this additional third parameter I think.

createCoreSetupMock may need to be modified so that you could pass mock return values to getStartServices

Proposed implementation SGTM

[stacey-gammon]

The other big downside to using the getter/setter pattern is that it looks very easy for a new dev to accidentally expose a static function that uses these helpers not realizing they are stateful.

++ to reusing the coreStartServices approach.

To help with the props, could you just have one additional prop that is an async getServices fn?

[mattkime]

Is this use case basically an internal circular dependency? I wonder if we're finding a clever way to maintain a bad code smell. The code that expresses this problem - is it necessary that it exhibit the problem?

--

That aside -

Would it make sense to create a new function instead of adding to getStartServices? Like getOwnStartServices, etc.

[lukeelmers]

If we want to propagate all the required services from the top level, we would have to add them to all of our props.

This is what KibanaContextProvider can be used for though, no?

The other big downside to using the getter/setter pattern is that it looks very easy for a new dev to accidentally expose a static function that uses these helpers not realizing they are stateful.

++ Yes, this is a big consideration. IMO the convenience of being able to import something stateful from a magical services.ts file means an increased risk of inadvertently using stateful code where you shouldn't. I prefer the explicitness of passing down stateful dependencies.

Is this use case basically an internal circular dependency?

It is not technically a circular dependency in that the use case here is code which is run _after_ start, but must still be registered _inside_ setup. So as long as you aren't trying to rely on that service being there in order for setup to return, it should be okay. The PluginWrapper code probably explains this more clearly than I can:
https://github.com/elastic/kibana/blob/2d3698972b80d22ac258b3d79f775037279026b4/src/core/public/plugins/plugin.ts#L121-L131

The startContract is already available for your plugin at the time startDependencies$.next is called (and this is ultimately what becomes the returned value of getStartServices). So it's mostly a matter of making sure startContract is passed along as well.

Would it make sense to create a new function instead of adding to getStartServices? Like getOwnStartServices, etc.

So still a core method, but just a separate one? That's certainly an option too; my thinking in updating the existing one is that folks could simply ignore the third item in the array if they don't need it, meaning the current usage still remains unchanged / non-breaking:

// most plugins:
const [coreStart, depsStart] = await core.getStartServices();
// plugins who want to receive their own contract:
const [coreStart, depsStart, selfStart] = await core.getStartServices();

[mattkime]

Makes sense to me.

[joshdover]

Extending getStartServices seems much simpler and safer than the other options 👍

1. More complex CoreSetup generic. As @pgayvallet pointed out, this would require CoreSetup to have another parameter as it would need to be aware of a plugin's own startContract:

interface CoreSetup<TPluginsStart extends object = object, TSelfStart extends object = object> {
  // ...
  getStartServices(): Promise<[CoreStart, TPluginsStart, TSelfStart]>;
}

I was trying to come up with a way to simplify this, but I can't think of anything great. Here are some alternatives in case someone prefers these, but I don't think they significantly change much:

Move generic to getStartServices:

interface CoreSetup<TPluginsStart extends object = object> {
  // ...
  getStartServices<
    TSelfStart extends object = object
  >(): Promise<[CoreStart, TPluginsStart, TSelfStart]>;
}

Allow a generic that is an extension of TPluginsStart:

interface CoreSetup<TPluginsStart extends object = object> {
  // ...
  getStartServices<
    TPluginsWithSelfStart extends TPluginsStart = TPluginsStart
  >(): Promise<[CoreStart, TPluginsWithSelfStart]>;
}

I think I still prefer the original proposal above for clarity and correctness.

[lukeelmers]

Here are some alternatives in case someone prefers these, but I don't think they significantly change much

I have no strong feelings either way on each of these approaches. TBH I don't think I even realized CoreSetup accepted a TPluginsStart parameter in the first place 😄

[pgayvallet]

TBH I don't think I even realized CoreSetup accepted a TPluginsStart parameter in the first place

It does only for getStartServices return type.

[pgayvallet]

I was trying to come up with a way to simplify this, but I can't think of anything great. Here are some alternatives in case someone prefers these, but I don't think they significantly change much

Both alternative have non-inferable TSelfStart typing, forcing to call getStartServices with the expected SelfStart type, where the initial proposal allow to have it implicit by adapting CoreSetup and Plugin, which is why I think it's preferable.

export interface Plugin<
  TSetup = void,
  TStart = void,
  TPluginsSetup extends object = object,
  TPluginsStart extends object = object
> {
  setup(core: CoreSetup<TPluginsStart, /* added */ TStart>, plugins: TPluginsSetup): TSetup | Promise<TSetup>;
  start(core: CoreStart, plugins: TPluginsStart): TStart | Promise<TStart>;
  stop?(): void;
}

[lukeelmers]

One other important difference between the createGetterSetter approach and getStartServices which we discussed today is that createGetterSetter is synchronous -- so in situations where it might not be possible to use the async getStartServices without significant refactoring, we may still need to lean on createGetterSetter in the interim.

That said, now that getStartServices is going to provide everything we would need, we can start designing with async in mind in the future, as we work toward adopting that solution more widely and refactoring our code over time.

[joshdover]

createGetterSetter is synchronous -- so in situations where it might not be possible to use the async getStartServices without significant refactoring, we may still need to lean on createGetterSetter in the interim.

Could you provide an example where this being synchronous is useful? I imagine in most (all?) cases that if this is not populated yet, you can't do whatever it is you need to do with the service, resulting in an error. Is it preferable to throw an exception or return a promise that will resolve once the operation _can_ be completed?

If you do still need to have a synchronous API, we could change this to an Observable which has the advantage that subscribers are executed immediately if there is a value (Promises are always on the next tick, so I don't think we can do the same a Promise):

let startServices;
core.getStartServices$()
  // this callback should be executed immediately if a value is already available
  .subscribe(services => (startServices = services));

// emulate previous behavior from createGetterSetter
if (!startServices) {
  throw new Error()
}

[lukeelmers]

Could you provide an example where this being synchronous is useful?

Not sure if this is the best example, but one that @alexwizp and I discussed today are the various agg types which get added to a registry during setup so they can be used in start.

Each registered agg type is provided as a class which extends a base class and has various methods which are exposed on the agg, some of which will need to use start services. Take, for example, the date_histogram agg type: https://github.com/elastic/kibana/blob/5d428b0d806160896b7e4ae3cc2fb9b7c5bf808d/src/plugins/data/public/search/aggs/buckets/date_histogram.ts#L111-L124

This getFormat method is not async, so when switching from the synchronous getFieldFormats() to using await core.getStartServices(), we'd need to make this method async too and update any places that call it.

I haven't done a full audit on this particular method -- it may not be that bad to change it around, but hopefully you get the gist.

If you do still need to have a synchronous API, we could change this to an Observable which has the advantage that subscribers are executed immediately if there is a value

++ I like this solution much more than createGetterSetter, and should solve the use case I describe above for situations where it isn't possible to immediately go async. In those cases we could just use the observable instead.

[alexwizp]

Could you provide an example where this being synchronous is useful?
I've tried to collect some examples where we use it in synchronous way in the data plugin

• _file_: src/plugins/data/public/ui/shard_failure_modal/shard_failure_open_modal_button.tsx
  _service_: CoreStart['overlays']
  _note_: React component (functional) - inside callback function
• _file_: src/plugins/data/public/ui/filter_bar/filter_item.tsx
  _service_: NotificationsStart
  _note_: React component (class) - inside render method
• files:
  src/plugins/data/public/search/aggs/param_types/field.ts
src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts
src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts
  _service_: NotificationsStart
  _note_: inside the constructor of FieldParamType`. The constructor cannot be asynchronous

and so on....

[pgayvallet]

If you do still need to have a synchronous API, we could change this to an Observable which has the advantage that subscribers are executed immediately if there is a value

I'm not a big fan of that kind of approach, as imho this is more of a trick usage by knowing that the consumed observable have a replay effect and that the underlying rxjs implementation execute handlers in a synchronous way when emitting (which I'm not even sure is documented / specified (please correct me here), even if we relies on that a lot on some part of the code), but this is a working workaround I guess.

(please feel free to invalidate any or all of that:)

src/plugins/data/public/ui/filter_bar/filter_item.tsx

For react components, I don't really see any good arguments TBH

I understand that this is just 'easier' to use static module accessors to retrieve these services when needed, however imho this is just an anti-pattern of react philosophy, and as any component rendering is ensured to be done after the whole platform start cycle (and as every render core handlers are async atm), that should 'easily' be replaced by either Props-based propagation (by awaiting start in the render method to get the service before the rendering) or a react ContextProvider.

src/plugins/data/public/search/aggs/param_types/field.ts

note: inside the constructor of FieldParamType`. The constructor cannot be asynchronous

The usage is not really inside the constructor, as it's used inside the deserialize method that is constructed inside the constructor. Also as the call to getNotifications().toasts.addDanger is not used in any synchronous way (I mean the call return is not used), replacing it with a promise-based approach like getNotificationPromise().then(notif => notif.toasts.addDanger(...) should be straightforward. The biggest issue I see there is more that the FieldParamType constructor got no parameters for these services, that are statically imported instead (stateful imports). But this is the root issue, isn't it.

More globally, the getXXXX accessors will only have their associated setter called after start. Meaning that they won't be populated/usable any sooner than the getStartService or the start cycle resolves anyway. I don't have enough context on all the provided example usages to have a definitive opinion, but I feel like properly awaiting start or getStartServices on a very higher level should be able to replace these module-statics with the resolved, and therefor 'synchronous', services (of course, it is always way easier to say it than to do it)

[ppisljar]

this pretty much means we cannot have _any_ sync method with dependency on start contracts on _any_ items that we add to _any_ registry.

its true that the getxxx accessors won't be available any sooner, but we can assume they are available which allows us to leave some code sync and avoid more refactoring.

we know that they were set as we only allow registration to our registries in setup but we only allow consuming them after start (registry get method is only exposed on the start contract).

[pgayvallet]

Initial issue fixed, however I'm reopening to continue the discussion

[lukeelmers]

Cross-posting @streamich's idea for a synchronous way of interacting with getStartServices:
https://github.com/elastic/kibana/pull/61413

[rudolf]

I'm not a big fan of that kind of approach, as imho this is more of a trick usage by knowing that the consumed observable have a replay effect and that the underlying rxjs implementation execute handlers in a synchronous way when emitting (which I'm not even sure is documented / specified (please correct me here)

This "trick" does require a value to have been emitted before being subscribed to, but the behaviour of subscribe handlers executing synchronously is part of the Observables spec. So I think the biggest disadvantage is that the code reads like async code, but is in fact depended upon to be sync which isn't very explicit.

In many ways this is like setting a class property in setup and then reading it in start, it's guaranteed to work, but the code doesn't make this explicit. It's also maybe brittle in that a change that seems unrelated can break the behaviour, although throwing as a precaution should quickly surface that.

I'll believe #61413 is possible when I see green CI 😛 , the spec definitely says .then callbacks should happen asynchronously (in a microtask).

[alexwizp]

@lukeelmers I think we can make it easier, without making synchronous the core.getStartServices method. Please see my proposal: #61628

[rudolf]

61413 Also doesn't take into account that we don't yet have synchronous lifecycles. So a plugin could do

start() {return new Promise(resolve => setTimeout(() => resolve({myStartApiMethod: () => ...}, 1000));

Once lifecycles are fully synchronous, we can build all the plugins' start contracts synchronously and therefore make getStartServices a synchronous function. This function will throw an exception if called before the start lifecycle finished, so it still has the same drawback as the other techniques like using an Observable or setting a class property: it's still possible to write code that throws an "start services not ready" type of exception. What it does provide is a cleaner API.

I believe this "start services not ready" exception problem is fundamental. Even though lifecycles are synchronous, setup and start don't happen during the same event-loop. This is because Core runs several async tasks after setup and before plugins start, the most obvious example is saved object migrations.

Unless we can make all the other core start services sync https://github.com/elastic/kibana/blob/4b012fb21af603efcf07c285f1c713e8eae6b828/src/core/server/server.ts#L180-L207

it will always be possible to write code that throws this exception with code like

setup(core) {
  setImmediate(() => {core.getStartServices()});
  process.nextTick(() => {core.getStartServices()});
  Promise.resolve().then(() => {core.getStartServices()});
}

If we want to eliminate these we either have to make Core's start fully synchronous or go back to async lifecycles. Make Core's start fully sync, if it's possible, will require at least moving the registering of saved object migrations into a pre-setup lifecycle or make them static.

[pgayvallet]

I'll believe #61413 is possible when I see green CI 😛 , the spec definitely says .then callbacks should happen asynchronously (in a microtask).

Yea, +1 for that

getStartServicesSync().core.anything should throw an NPE atm, as the function execution and .anything access should be performed in the same task (according to the spec) .

We discussed at some point to have something like getStartServicesSyncOrDie() that would return the start services in a synchronous way, and just throw if called before core start execution, but we choose to implements the async getStartServices() instead, as it felt safer. If this sync accessor is really a need that solve or simplify things, maybe we should reopen the discussion about that sync getter.

[lukeelmers]

I think we can make it easier, without making synchronous the core.getStartServices method. Please see my proposal: #61628

@alexwizp I commented on the PR, but I think your proposal there is a step in the right direction in the interim (even if the long term solution ends up being different), as it will consolidate our usage of the getter/setter pattern, making it easier to change down the road.

That said, I still don't love the idea of createGetterSetter being the pattern we use to solve this problem in the long run.

Since we've established that there's still a need for accessing start services synchronously, IMHO the ideal outcome would be for core to provide an official way of doing this, rather than leaving it up to plugins. core.getStartServices$ or core.getStartServicesOrDie both seem like possibilities here.

[streamich]

+1 for core providing getStartServicesOrDie and ideally it would return a named object, like

getStartServicesOrDie().core
getStartServicesOrDie().plugins

instead of

getStartServicesOrDie()[0]
getStartServicesOrDie()[1]

in plugin code we could call it start for short and pass it around, like

start().plugins.uiActions.getTrigger('RANGE_BRUSH_TRIGGER').exec({})

core.getStartServices$ does not seem to be useful to me, to effectively use it we would need to transform it into getStartServicesOrDie ourselves.

Alternatively, if Platform team does not want to own the getStartServicesOrDie function, we can implement it in kibana_utils with minor modifications to getStartServicesSync in my PR. Reason being that getStartServicesOrDie is an improvement over createGetterSetter.

[pgayvallet]

We discussed about this issue in our team weekly yesterday.

The conclusion was that even if none of the listed options seems like good enough for a long term solution, the sync getStartServicesOrDie looked like the best quick win in the short term. We'll see if we can think of any better option this week, and will deliver a final decision for the short term during our next weekly.

and ideally it would return a named object, like getStartServicesOrDie().core

The array type would allow the same thing with start()[1].uiActions.getTrigger('RANGE_BRUSH_TRIGGER').exec({}), even if I have to agree that the object type is way more readable and explicit.

I think the return type should be an object. My only concern is consistency. getStartService() and getStartServicesSync() should return the same types, so probably that both should be changed to object return, but ideally at the same time, so either it should be done at a later time, or getStartService should be changed before introducing getStartServicesSync

[streamich]

It could returns both ways initially

await getStartServices()[0] === await getStartServices().core

with the following work that deprecates

await getStartServices()[0]

[pgayvallet]

It could returns both ways initially

Yea, but changing the getStartServices return type to support both [0] and .core access at the same time would breaks dozen of usages / TS checks, mostly in tests due to mocked return values no longer matching that (see https://github.com/elastic/kibana/pull/61216's diff for instance) so this transition wouldn't really help / be faster than just replacing this array return to an object 'directly'

[restrry]

as we Prefer one way to do things shouldn't we adopt getStartServices to getStartServicesOrDie interface?
tbh I don't see how it's different from implementing it locally

// before
setup(core, plugins){
   plugins.pluginA.register(async () => {
     const plugins = await core.getStartServices()[1];
     plugins.pluginA.doSomething()
   }); 
}

// after
setup(core, plugins){
   plugins.pluginA.register(() => {
     const { plugins } = this.startDeps!;
     plugins.pluginA.doSomething()
   }); 
}
start(core, plugins) {
  this.startDeps = { core, plugins };
}

The other alternatives I can think of, requiring changing interface of lifecycles (expose the whole plugin interface at once, for example), removing explicit lifecycles, provide but we lose predictability of the current implementation in this case.

[pgayvallet]

In cases where your before implem was a commonly used pattern (async registering registries), I think there wouldn't be any real problem in using current implementation of core.getStartServices

The issue is that most of the registries shared between plugins are currently synchronous, and only accept to register resolved objects in a synchronous way, and that during setup.

An example of reason to use the sync module-getter trick would be:

// before

// some file
import { getModuleStaticService, setModuleStaticService } from './service_getters'

class SomeProvider {
  doSomethingSync() {
    // will always been executed after `start` anyway
    getModuleStaticService().doStuff();
  }
}

// plugin file
Plugin {
  setup(core, {pluginA}){
    pluginA.registerSync(new SomeProvider());
  }

  start(core) {
    setModuleStaticService(core.someService);
  }
}

The introduction of getStartServicesSync / getStartServicesOrDie would allow to remove the module static trick (even if just moving it to core technically)

// after

class SomeProvider {
  constructor(private serviceGetter: () => SomeService) {}
  doSomethingSync() {
    serviceGetter().doStuff();
  }
}

setup(core, {pluginA}){
  const serviceGetter = () => core.getStartServicesOrDie().someService;
  pluginA.register(new SomeProvider(serviceGetter));
}

Although, you solution would also work, even if it still forces to use a 'visible' ! cast and a local plugin property to use:

class Plugin {
  private startDeps?: CoreAndDeps

  setup(core, {pluginA}){
    const serviceGetter = () => this.startDeps![0].somePlugin;
    pluginA.register(new SomeProvider(serviceGetter));
  }

  start(core, plugins) {
    this.startDeps = { core, plugins };
  }
}

It's still quite similar technically to what we can provide with a sync core getter. It just feels a little less 'integrated' in core's framework imho.

One other option would just be to declare and document that async-registering registries should be the way to go in NP.

Something like

class SomeRegistry {
  private pendingRegistrations: Array<Promise<SomeStuff>> = [];
  private entries: SomeStuff[] = [];

  setup() {
    return {
      register: (Promise<SomeStuff>) => pendingRegistrations.push(pendingRegistrations);
    }
  }

  async start() {
    const stuffs = await Promise.all(this.pendingRegistrations);
    stuffs.forEach(stuff => entries.push(stuff));

    return {
      getAll: () => entries,
    }
  }
}

Upsides

• Basically, I think it addresses the root issue

Downsides

• Registry implem is more complex
• can't do any registration validation duringregister (i.e check if an item has been registered twice), all registry entries validation have to move to start, when we actually resolves the promises.

Limitations:

• It depends on the plugin exposing the registry to follow this pattern. If pluginA is still exposing a synchronous registry, and your plugin is the first one to need to register something async / depending on start services, you will be blocked until pluginA refactors its registry.
• It requires plugins start to be asynchronous to allow resolving the registration promises. This go against the sync lifecycle RFC and is therefor probably a blocker.

Personally, to have tried to implement that in the SavedObjectsManagement plugin, I did not feel convinced that this was a solid option.

[joshdover]

Notes from our meeting:

Why this lifecycle & dependency problem exists:

• We need to know when certain things are completed or registered like HTTP routes and SO migrations
• We want registrations to be dynamic to be more flexible and avoid complexity of providing a system for plugins to read static registration for extension points.
• We need sync lifecycles b/c we don't want a plugin to be able to stop Kibana from starting up / hanging.

Decision:

• Move forward with documenting the manual approach above (https://github.com/elastic/kibana/issues/61106#issuecomment-607842767)
  
  
  
  • Reasons: we prefer for this synchronous usage to not proliferate in the code base so we do not want to provide an explicit API for this. The manual implementation is also simpler to understand.
  
  
• Revisit the fundamental problems with lifecycles & dependencies in the future, after all plugins have migrated.

[streamich]

This "manual" approach does not seem useful at all, it only can be used if your code that you are registering is defined inline in the plugin, if the actual registrants are defined in other modules—like it is done in most places in Kibana—this pattern will simply require everyone to implement getStartServiceOrDie in every plugin.

This will not work:

setup(core, plugins){
   plugins.pluginA.register(myRegistrant(this.startDeps!)); 
}
start(core, plugins) {
  this.startDeps = { core, plugins };
}

Following this "manual" approach, to make it work, the plugin basically needs to implement the getStartServicesOrDie itself:

getStartServicesOrDie = () => {
  if (!this.startDeps) throw 123;
  return this.startDeps;
};
setup(core, plugins){
   plugins.pluginA.register(myRegistrant(this.getStartServicesOrDie)); 
}
start(core, plugins) {
  this.startDeps = { core, plugins };
}

Not sure how useful it is to document this "manual" approach.

For us it seems the best approach I can see now is to add getStartServicesOrDie function to kibana_utils plugins, so we don't have to implement it in every plugin.

[lukeelmers]

we prefer for this synchronous usage to not proliferate in the code base so we do not want to provide an explicit API for this.

That's fair & I can get on board with this stance -- However, I do think it's worth pointing out that as long as there are synchronous items being added to registries in Kibana, people will be devising their own workarounds for this whether we like it or not.

If we truly want to discourage this usage, we should probably be giving guidance around how folks should design registries to avoid needing sync access to start services in the first place. Otherwise this will be a recurring problem.

For us it seems the best approach I can see now is to add getStartServicesOrDie function to kibana_utils plugins, so we don't have to implement it in every plugin.

If we are taking the stance that we don't want folks accessing start services synchronously, then IMO putting this in kibana_utils is effectively the same as putting it in core, unless we are very carefully documenting "here's a tool for something you can do that we don't want you to do but you'll probably have to do anyway"

At this point my vote would be to pick a pattern for dealing with this and implement it in each plugin individually, as we are only talking about ~3-5 lines of added code.

I think @streamich's proposal would work, and is similar to the one @alexwizp had in #61628. To me the biggest benefits are 1) avoiding importing from modules like ./services so that folks are required to actually pass this dependency down to their application code, and 2) Having all of this handled in the Plugin class, which makes it easier to reason about.

[pgayvallet]

If we truly want to discourage this usage, we should probably be giving guidance around how folks should design registries to avoid needing sync access to start services in the first place. Otherwise this will be a recurring problem.

The team is on the same page here. We definitely should be doing that. BTH my async registry example in https://github.com/elastic/kibana/issues/61106#issuecomment-608258536 seems to be an option, however this kinda go against the whole sync lifecycle plan, which is why we need to think about that with a wider vision. Also most options include some breaking changes in the core APIs, which is why we don't want to tackle that before 8.0 to avoid introducing factors that will slow down the NP migration for plugins. This is the main reason for the 'lets wait' decision, even if far from perfect.

This "manual" approach does not seem useful at all, it only can be used if your code that you are registering is defined inline in the plugin, if the actual registrants are defined in other modules—like it is done in most places in Kibana—this pattern will simply require everyone to implement getStartServiceOrDie in every plugin.

I agree that this is a risk.

The async -> sync bridge is quite straightforward:

type StartServicesSyncAccessor<
  TPluginsStart extends object = object,
  TStart = unknown
> = () => UnwrapPromise<ReturnType<StartServicesAccessor<TPluginsStart, TStart>>>;

// open to naming, `convertToSyncStartServicesAccessor`?
const getStartServicesSyncFactory = <TPluginsStart extends object, TStart>(
  accessor: StartServicesAccessor<TPluginsStart, TStart>
): StartServicesSyncAccessor<TPluginsStart, TStart> => {
  let contracts: [CoreStart, TPluginsStart, TStart] | undefined;

  accessor().then(asyncContracts => {
    contracts = asyncContracts;
  });

  return () => {
    if (contracts === undefined) {
      throw new Error('trying to access start services before start.'); // or return undefined?
    }
    return contracts;
  };
};

Maybe without exposing a startService sync accessor as an 'official' API from coreSetup, we should still have this converter exposed as a static util from core for plugins to be able to use it when needed instead of having every plugin implementing their own version? At least as a temporary measure until we tackle step2 (Revisit the fundamental problems with lifecycles & dependencies in the future, after all plugins have migrated.)

@elastic/kibana-platform WDYT? Would exposing the sync converter as a static util be acceptable, or do this go against the we prefer for this synchronous usage to not proliferate in the code base so we do not want to provide an explicit API for this part?

[ppisljar]

catching up to this discussion and one question comes to mind: @rudolf is above mentioning that at the moment lifecycle methods can still be async, and that once all of them are sync getStartServices can be sync as well ?

what will actually happen in this case then, if setup can be async ?

async setup() {
   const startServices = await core.getStartServices();
   return {
    ....mySetupContract
   }
}

the setup life cycle will never complete, as we'll be waiting for start lifecycle to complete which should not start until setup is done ?

and which plugins actually have async setup methods ? is this something we can refactor soon ?

[rudolf]

the setup life cycle will never complete, as we'll be waiting for start lifecycle to complete which should not start until setup is done ?

That's correct. It's equivalent to the synchronous version in that they both break Kibana. The one will die with an exception, the other hang and never resolve:

setup() {
   const startServices = core.getStartServicesOrDie(); // always throws
   return {
    ....mySetupContract
   }
}

and which plugins actually have async setup methods ? is this something we can refactor soon ?

We're tracking the work in https://github.com/elastic/kibana/issues/53268 but the list of plugins that need refactoring is incomplete. We initially audited all plugins during the Lifecycles RFC and it didn't seem like there were any plugins that required more than a bit of refactoring after we expose a way to read config synchronously. But we haven't done a more recent audit...

[lukeelmers]

FYI to those following along, @streamich has a PR up to add a getStartServicesOrDie util which we will plan to use in absence of having a sync way of accessing these in core. It's pretty much the same implementation @pgayvallet outlines above.

we prefer for this synchronous usage to not proliferate in the code base so we do not want to provide an explicit API for this.

@joshdover When we were discussing this as a team, one question that came up was around potential performance repercussions of preferring to access start services asynchronously. As we dove into that topic, we realized that we didn't have a great understanding of why you'd prefer to avoid synchronous usage -- would you be able to expand on this a little so that we have a better grasp of the tradeoffs for future reference?

[joshdover]

we realized that we didn't have a great understanding of why you'd prefer to avoid synchronous usage -- would you be able to expand on this a little so that we have a better grasp of the tradeoffs for future reference?

The main reason against preferring a sync accessor is that it requires the developer to understand and reason about _when_ that function can be called. Having an API that fails sometimes is not what would I consider a good API design. It also makes accurately testing this difficult.

Promises solve this problem by being able to defer execution until the start services are available, which allows the consumer of the API to not need to know underlying implementation details of the API. Though as @ppisljar pointed out above, this isn't quite true since it's possible for a developer to create a situation that would never resolve.

In practice, this would trigger the lifecycle timeout that Core implements, resulting in crashing the Kibana server or client application. I could entertain an argument that this failure scenario is actually a _worse_ developer experience than a sync accessor that can fail, since the cause of the timeout is not immediately obvious. If there is consensus that that statement is true, then I'd be comfortable introducing this into Core.

I think this entire problem is a code smell that indicates fundamental issues with the general design of the Kibana Platform. We introduced multiple lifecycle methods in order to avoid having to make all of Kibana reactive. However, there are clearly scenarios where this pattern doesn't work well. This is a complicated problem and I think we need to spend dedicated time on solving this issue more holistically. However, I don't think now is the right time do that while so many plugins are still migrating.

[lukeelmers]

Having an API that fails sometimes is not what would I consider a good API design. It also makes accurately testing this difficult.

I think this entire problem is a code smell that indicates fundamental issues with the general design of the Kibana Platform.

This is a complicated problem and I think we need to spend dedicated time on solving this issue more holistically. However, I don't think now is the right time do that while so many plugins are still migrating.

++ Agreed on these points, including that we should revisit this discussion later -- perhaps as the planning around sync lifecycle methods progresses. Thanks for clarifying!

In the meantime, I think #63720 should work just fine for us, and is lightweight enough that if a core solution is introduced later it shouldn't be a huge effort to migrate things over to it.

On that note, I'll go ahead and close this issue as I think we have achieved the original goal of a better solution to this problem than createGetterSetter. Folks can feel free to chime in still if there's anything further to add, otherwise we can pick this discussion back up a bit further down the road.