[Search] fix cancelation related memory leaks #81996
Conversation
Dosant
added
Feature:Search
Dosant
changed the title
Dosant
requested review from
lizozom and
lukasolson
and removed request for
lizozom
| @@ -59,7 +59,7 @@ describe('ExecutionContract', () => { | |||
|
|
|||
| test('can cancel execution', () => { | |||
| const execution = createExecution('foo bar=123'); | |||
| const spy = jest.spyOn(execution, 'cancel'); | |||
| const spy = jest.spyOn(execution, 'cancel').mockImplementation(() => {}); | |||
There was a problem hiding this comment.
I noticed that test were silently failing tests (on master)
(node:57027) UnhandledPromiseRejectionWarning: AbortError: Aborted
(node:57027) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:57027) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
I assume that it warns is that cancel is called without start and there is no one subscribed to handled inner AbortRejection promise of the execution.
I mocked it, since this unit test for execution_contract and we are not testing an execution here.
There was a problem hiding this comment.
I just fixed that particular higher level test, but really, probably we should also handle and test edge case:
const ex = new Execution();
ex.cancel() // ?? what happens?
ex.start() // ?? what happens?
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
There was a problem hiding this comment.
I looked over these changes and while I'm not a huge fan of adding properties to built-in classes, I can't really think of much of a better way to accomplish this (other than what's suggested below, which I'm also not a huge fan of). Feel free to move forward with this approach unless you think the suggested approach feels cleaner.
| return new Promise((resolve, reject) => { | ||
| if (signal.aborted) reject(new AbortError()); | ||
| const abortHandler = () => { | ||
| export function toPromise(signal: AbortSignal): Promise<never> & { cleanup: () => void } { |
There was a problem hiding this comment.
Hmm, I'm not a huge fan of adding a method to a built-in class (Promise). We might consider returning an object instead (like { promise, cleanup }). I think it will also help to enforce that any consumers of this function will handle cleaning up and not just use the promise returned.
There was a problem hiding this comment.
I don't mind initial approach I took mostly because it is clearly typed that it has a cleanup method, nevertheless:
I think it will also help to enforce that any consumers of this function will handle cleaning up and not just use the promise returned.
I agree with this 👍 changed {promise, cleanup}
one tiny caveat is that now we can't just use:
Promise.race(signals.map(toPromise))
but have to:
Promise.race(signals.map(toPromise).map(pr => pr.promise))
| const cleanup = () => { | ||
| subscription.unsubscribe(); | ||
| combinedController.signal.removeEventListener('abort', cleanup); | ||
| combinedController.abort(); |
There was a problem hiding this comment.
Is the reason we need to abort here because we need to trigger the internal cleanup from getCombinedController? If so, maybe we can do similarly to what I've suggested above (instead of returning just the signal from getCombinedSignal, return something like {signal, cleanup}). Thoughts?
There was a problem hiding this comment.
Is the reason we need to abort here because we need to trigger the internal cleanup from getCombinedController
Yes, exactly.
If so, maybe we can do similarly to what I've suggested above (instead of returning just the signal from getCombinedSignal, return something like {signal, cleanup})
Agree. Done!
botelastic
bot
added
the
Feature:ExpressionLanguage
💚 Build SucceededMetrics [docs]page load bundle size
History
To update your PR or re-run it, just comment with: |
* master: Add derivative function (elastic#81178) [Discover] Deangularize context_app.html, part 3 (elastic#81838) [Visualize] Vis listing page breaks on unknown vis type (elastic#82018) Rename `batchSize` parameter to `batch_size` to be consisten with the API namings guidelines. (elastic#82123) Minor edits in Single Metric Viewer (elastic#82159) [Actions] Fix type contract (elastic#82168) Upgrade EUI to v30.1.1 (elastic#81499) Skip failing ES snapshot test (elastic#82207) Skip ES snapshot failing suite (elastic#82206) [Alerting UI] Grouped list of alert types using producers in Types filter of Alerts tab (elastic#81876) [Maps] convert vector style component to typescript round 1 (elastic#81961) Fix link to upgrade assistant (elastic#82138) Rename "service overview" to "service inventory" (elastic#81933) adjust policy test to drop test for server addresses (elastic#82120) Cleanup/codeowners (elastic#82146) [DOCS] Updates add data content (elastic#81093) [DOCS] Remove index mgmt docs (elastic#82099) [Search] fix cancelation related memory leaks (elastic#81996)
Summary
Attempts to fix #65051
See explanation: #65051 (comment)
For my experiments with branch the leak is fixed.
Checklist
Delete any items that are not applicable to this PR.