Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[plugins] Log warnings from core in dev mode when setup contracts are called after start. #124039

Open
lukeelmers opened this issue Jan 28, 2022 · 4 comments
Labels
Feature:Plugins impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@lukeelmers
Copy link
Member

lukeelmers commented Jan 28, 2022

There are many places in Kibana where we find plugins abusing lifecycle methods by doing things like holding references to dependencies from the setup contract, and then using that dependency later during the start lifecycle.

While some of these abuses are relatively harmless, it can create much more serious problems in the case of things like registries: If a plugin holds a reference to a registry from setup, and then registers an item to it during start, plugins can no longer be assured that the registry is fully populated by the time we reach start... thus breaking the guarantees lifecycles were designed to provide.

There are some places in core where we have put safeguards in place to prevent such behavior:

registerType: (type) => {
if (this.started) {
throw new Error('cannot call `registerType` after service startup.');
}

However, the only way to have this be completely bulletproof would be for every plugin exposing public registries to apply similar safeguards. A much simpler way to start would be by simply logging a warning while in dev mode if we detect a plugin calling any setup contracts after start has begun.

Tasks

  • Wrap every lifecycle API from core, and perform a check to see if the cycle is active
  • If setup API has been called in start or stop, and we are in dev mode, log a warning
  • If start API has been called in stop, and we are in dev mode, log a warning

cc @stacey-gammon

@lukeelmers lukeelmers added Feature:Plugins Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc loe:small Small Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Jan 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@Dosant
Copy link
Contributor

Dosant commented Jan 28, 2022

Since ideally we also need this for each plugin, I am curious if you plan to:

  1. have a one-off core internal solution for now,
  2. or if core would also automatically handle this for all the plugins. (proxying/wrapping each plugin's contract?)
  3. or have a shared utility that plugins can use to wrap their setup contracts that will track when something is accessed

@pgayvallet
Copy link
Contributor

I would love to have a generic wrapping solution to be honest, either automatically performed inside core, or exposed as an utility for plugins to use, as adding an explicit check, or manually wrapping all APIs will be way too verbose.

However, given the nested (and sometime dynamic) nature of core's and our plugins' lifecycle contracts, I'm not 100% sure this can easily be performed automatically, unless we implement some kind of deep proxy wrapping.

e.g

setup(core) {
   this.deepRef = core.someService.someRegistry
}

start() {
   this.deepRef.shouldntBeCallingThatDuringStart()
}

Would not be caught by a basic proxy approach wrapping either coreSetup or even each individual service.

Also note that for performances reasons, we probably want to perform this wrapping only in dev mode (silencing the warning in prod is not good enough).

@Dosant
Copy link
Contributor

Dosant commented Jan 31, 2022

unless we implement some kind of deep proxy wrapping.
we probably want to perform this wrapping only in dev mode

Maybe if we indeed do it only in dev mode and exclude performance concerns, then deep proxy does make sense and is doable here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants