Fix deadloop in case of NP circular dependencies #47933
Fix deadloop in case of NP circular dependencies #47933pgayvallet merged 7 commits intoelastic:masterfrom
Conversation
Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
|
Pinging @elastic/kibana-platform (Team:Platform) |
💚 Build Succeeded |
|
|
||
| await expect(pluginsSystem.setupPlugins(setupDeps)).rejects.toMatchInlineSnapshot( | ||
| `[Error: Topological ordering of plugins did not complete, these edges could not be ordered: [["some-id",{}]]]` | ||
| `[Error: Topological ordering of plugins did not complete, cyclic dependency detected between: ["some-id"]]` |
There was a problem hiding this comment.
That's a weird message for a missing required dependency case
There was a problem hiding this comment.
No, this message is for cyclic dependencies, not missing ones.
There was a problem hiding this comment.
but we test here.
`setupPlugins` throws plugin has missing required dependency`
if we assume that we cannot have missing deps we should remove this test
There was a problem hiding this comment.
I apologize, you are actually right. The error can also be thrown in case of missing dependencies.
However, in real case usage, it will never occurs as PluginService#shouldEnablePlugin will return false and therefor not adding the plugin to the pluginSystem, so PluginSystem#getTopologicallySortedPluginNames will never be called with missing deps.
So not sure how what is the best thing to do here. Should I remove the unit test because it's testing a non-existing scenario, or should I put a more generic error message like these plugins have cyclic or missing dependencies: ${edgesLeft}
I think the second option is still better. @restrry wdyt ?
|
|
||
| await expect(pluginsSystem.setupPlugins(setupDeps)).rejects.toMatchInlineSnapshot( | ||
| `[Error: Topological ordering of plugins did not complete, these edges could not be ordered: [["depends-on-1",{}],["depends-on-2",{}]]]` | ||
| `[Error: Topological ordering of plugins did not complete, cyclic dependency detected between: ["depends-on-1","depends-on-2"]]` |
There was a problem hiding this comment.
Can we guarantee that they have circular dependencies? Probably both have missing deps.
There was a problem hiding this comment.
This is the pluginSystem sorting algorithm. Plugin with missing dependencies are already evicted at this point. Unsorted Kahn Graph is pretty much a cyclic dependency yes. (but I only changed the error message here)
Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
💚 Build Succeeded |
mshustov
left a comment
There was a problem hiding this comment.
still wait for the test to be updated/removed. #47933 (comment)
Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
💚 Build Succeeded |
💚 Build Succeeded |
* fix deadloop in case of cyclic dependencies Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * better error message in case of cyclic plugin dependency Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add createPlugin helper for readability Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add more complex test on discover cyclic deps Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * better error message Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
* fix deadloop in case of cyclic dependencies Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * better error message in case of cyclic plugin dependency Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add createPlugin helper for readability Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add more complex test on discover cyclic deps Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * better error message Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>
Summary
RangeError: Maximum call stack size exceededwhen NP plugins had circular dependencies within their required dependency graph. Now display the same, more explicit error message that when we encounter a cycle in the optional dependencies.Fixes #44762