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

Enforce lazy creation in RouteHandlerContext #84763

Closed
mshustov opened this issue Dec 2, 2020 · 2 comments · Fixed by #129896
Closed

Enforce lazy creation in RouteHandlerContext #84763

mshustov opened this issue Dec 2, 2020 · 2 comments · Fixed by #129896
Labels
enhancement New value added to drive a business result performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Dec 2, 2020

The current implementation of RouteHandlerContext extends context eagerly even if this API is not used. It creates unnecessary pressure on the CPU & Garbage collector. #78957 made core context provider lazy, but other providers are still created immediately.
Context core service should enforce instantiation on demand, which might require changing the IContextProvider interface.
For example, we can make IContextProvider function sync to call it lazily during access time.

core.http.registerRouteHandlerContext('pluginA', (context) => {
  return {
    ping: () => context.core.elasticsearch.legacy.client.callAsInternalUser('ping') as Promise<string>,
  }
});
// buildContext
Object.defineProperty(contextObject, contextId, {
  get() { create lazily }
});

The main reason why IContextProvider is async - some plugins access getCoreServices there

@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc performance enhancement New value added to drive a business result labels Dec 2, 2020
@elasticmachine
Copy link
Contributor

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

@pgayvallet
Copy link
Contributor

For example, we can make IContextProvider function sync to call it lazily during access time.

That seems like a pragmatic solution, but that would change the whole IContextProvider API signature, and this interface is also exposed by core's context service, both on client and server side, so we need to check the usages in the codebase.

Another (also breaking) API change to keep the async capabilities would be to change the buildContext / HandlerContextType signature:

private async buildContext(
source: symbol,
...contextArgs: HandlerParameters<THandler>
): Promise<HandlerContextType<THandler>> {

return { 
   ...resolvedContext, 
    [contextName]: await provider(exposedContext, ...contextArgs), 
    }; 

could become

return { 
     ...resolvedContext, 
    [contextName]: get: () => await provider(exposedContext, ...contextArgs),  // not the real syntax, would be using `defineProperty `
}; 

then

core.http.registerRouteHandlerContext('pluginA', (context) => {
  return {
    ping: () => context.core.elasticsearch.legacy.client.callAsInternalUser('ping') as Promise<string>,
  }
});

would be

core.http.registerRouteHandlerContext('pluginA', async (context) => {
  return {
    ping: () => (await context.core).elasticsearch.legacy.client.callAsInternalUser('ping') as Promise<string>,
  }
});

This would have a major impact on all IContextProvider usage though, so not sure this is a great option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants