[Search] Add cancel function to pollSearch

[lizozom]

Summary

Resolves #85603

Makes sure that Elasticsearch DELETE requests for async searches are properly sent on abort, even if consumer unsubscribes, both from client and server.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[lukasolson]

While playing around with this, I realized we have another issue with pollSearch. It doesn't defer, which means the search function will be called (just once) before there are any subscribers. Do you mind wrapping it in a defer and adding a test case for this?

[weltenwort]

The de-duplication of the cancel looks elegant. 👍

Unfortunately these changes don't quite make the triggering of the abortSignal and the unsubscription commutative. I left a suggestion for another variant in the comments.

(I'm using the Logs UI's log entry details flyout as a test-bed for verification.)

Thanks for following up on the issue!

[weltenwort]

Unfortunately this still prevents cancel call from being called if the consumer unsubscribes from the returned Observable before the abortSignal is triggered. The finalize unregisters the aborted.promise from the abortSignal.

How about the following?

export const pollSearch = <Response extends IKibanaSearchResponse>(
  search: () => Promise<Response>,
  cancel?: () => void,
  { pollInterval = 1000, abortSignal }: IAsyncSearchOptions = {}
): Observable<Response> => {
  return defer(() => {
    if (abortSignal?.aborted) {
      return EMPTY;
    }

    if (cancel && abortSignal) {
      abortSignal.addEventListener('abort', cancel, { once: true });
    }

    const aborted$ = (abortSignal ? fromEvent(abortSignal, 'abort') : EMPTY).pipe(
      map(() => {
        throw new AbortError();
      })
    );

    return from(search()).pipe(
      expand(() => timer(pollInterval).pipe(switchMap(search))),
      tap((response) => {
        if (isErrorResponse(response)) {
          throw response ? new Error('Received partial response') : new AbortError();
        }
      }),
      takeWhile<Response>(isPartialResponse, true),
      takeUntil<Response>(aborted$)
    );
  });
};

This seems to work and avoids an unnecessary intermediate Promise. What do you think?

[lukasolson]

Will this result in a memory leak since it is never removing the event listener from the abortSignal?

[weltenwort]

I would suspect not, because nothing holds a reference to the abort signal once the observable goes out of scope. The gc should be able to clean it up afterwards.

[weltenwort]

The de-duplication is neat, but we have to guard against calling this when the abortSignal has been triggered, because that has already caused the DELETE request to be sent.

Suggested change

	cancel();
	if (!combinedSignal.aborted) {
	cancel();
	}

Otherwise it will be sent twice on explicit abort.

[weltenwort]

@lukasolson are you still considering to fix this scenario?

[tsullivan]

Hi @lizozom,

Any tips on how reviewers can test this?

Any of the checklist items need to be checked off or removed?

[weltenwort]

@tsullivan I've been using the Logs UI "log entry details fly-out" to test:

It shows a progress bar, offers an explicit "cancel" button and cancels implicitly when the fly-out is closed while loading. It represents a more realistic usage with varying sequences of events (subscribe, unsubscribe, abort, ...) that unit tests don't cover.

[tsullivan]

I tested the code change in a plugin, and found that if the consumer unsubscribes the search observable early, then the DELETE request is not sent.

If the abort signal is called explicitly, then the DELETE request is sent twice as @weltenwort noted in their comment.

addressed

[lukasolson]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 9b8085f

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
dataEnhanced	27.4KB	27.4KB	+1.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
dataEnhanced	39.4KB	38.9KB	-477.0B

History

• 💚 Build #97788 succeeded 84fa8ee31e8baa5143798cb090dc44948b0bfb8a
• 💔 Build #97781 failed 8d5e2f4
• 💔 Build #97588 failed 0df0079
• 💚 Build #96047 succeeded 2016703
• 💚 Build #95120 succeeded 2016703

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[weltenwort]

The cancellation seems to work reliably in all situations I tested. Thanks for hanging in there! 👍

[weltenwort]

elegant 👍