Memory leak with errorHandlers

[stacey-gammon]

After trying to investigate some of the memory issues in our system I think I found one with errorHandlers in the courier.

This line right here: https://github.com/elastic/kibana/blob/master/src/ui/public/courier/data_source/_abstract.js#L163 pushes an object with a defer and a search source (which can be large) onto an errorHandlers array.

This errorHandlers array is only ever dealt with, as far as I can tell, in error_handler.handleError. Even there it doesn't seem to be removed from the array.

Running a request every 1 second for 20 visualizations in a dashboard led me here via heap snapshots. If I simply remove the

  errorHandlers.push({
    source: self,
    defer: defer
   });

portion of the code, my testing showed a dramatic decrease in leaked memory - from about 10MB to 1MB over a 60s period.

cc @spalger @weltenwort

[weltenwort]

Right, I totally forgot about that. I stumbled across this while creating the context view. I failed to find a way to reliably clean up the error handlers. As far as I could determine fixing it would require changing every code path that performs courier requests (and much of the internal queue/retry logic).

So I created fetchAsRejectablePromise as an alternate code path that avoids it completely.

Thanks a lot for pointing this out, @stacey-gammon!

[stacey-gammon]

Would it be possible to do something like have onResults remove the errorHandler? If we get the results then seems like there was no error so we won't need them to hang around any longer? In addition I think we'd have to remove them when we did see an error for that path to work.

Something like:

  SourceAbstract.prototype.onResults = function (handler) {
    const self = this;

    return new PromiseEmitter(function (resolve, reject) {
      const defer = Promise.defer();
      defer.promise.then((response) => {
        const returnValue = resolve(response);
        _.remove(errorHandlers, handler => handler.source === self);
        return returnValue;
      }, reject);

      self._createRequest(defer);
    }, handler);
  };

Otherwise, won't anyone who still uses fetch leak those errorHandlers?

[weltenwort]

I tried to recall my investigations from a few months ago with only limited success. There were problems with the various ways in which requests can be triggered and retried (not to mention the segmented request stuff), the order in which the error and success handlers were called, and... others. I will dive into this over the next days and update you with my findings. Thanks again! 😄

[weltenwort]

The problem with removing the error handler in onResult seems to be, that in some situations the PromiseEmitter "features" of the return value are relied upon as kind of an observable. It emits new values whenever courier.fetch() is called in some other code path. That means that there is no 1:1 correspondence between onResult and onError. If in those cases the error handlers are removed in the onResult callback, errors in subsequent requests are not caught and lead to the "fatal error" screen being shown.

Cleaning this up would mean to replace these courier.fetch() calls by some explicit signaling mechanism that triggers the main "reload data" code path in which proper error handler cleanup can be guaranteed.