"Cannot read property 'then' of null" since 7.14.0

[oguimbal]

💥 Regression Report

Since 7.14.0, we're seing Cannot read property 'then' of null errors in @elastic/elasticsearch/lib/Transport.js:167:18 .

May be related with #1516 ? (errors are both nullrefs, but not thrown from the same place)

Last working version

Worked up to version: 7.13.0

(fyi we mistakenly did not pin elasticsearch package minor version in our build pipeline, and we've seen that regression in prod, so i'm pretty sure that it has been OK with every version <= 7.13.0 for the past ~2 years)

Stopped working in version: 7.14.0

To Reproduce

I'm not sure... I dont have tried to reproduce it locally. But call .search() ? (that is the only thing that our code does, and our prod logs suggest that it was failing systematically)

[edit] See snipped here to reproduce

Expected behavior

No errors :)

Your Environment

Elastic Beanstalk running Node 10 (everything is else default)

[delvedor]

Hello! It does not look like it's related to #1516. Are you able to reproduce it in an isolated snippet of code? It would be very helpful!

[oguimbal]

Mmmh, in fact, there is a library involved aws-elasticsearch-connector (We're using our AWS credentials to connect to our cluster).

This means that I cannot reproduce the error locally (I dont have an AWS cluster that is accessible from my local machine, and the bug does not happen with my local cluster, which does not not involve AWS auth), but this is functionally equivalent to what we're doing (which is very simple... you know, for search 🙂)

import { Client } from '@elastic/elasticsearch';
import connector from 'aws-elasticsearch-connector';
import AWS from 'aws-sdk';

const cached = new Client({
    ...connector(AWS.config),
    nodes: ['...node addresses...'],
});

(async () => {
   // some query
   await cached.search({});
})();

That said, I'm not sure to see the point of this code when looking at that ... to summarize, you have :

1) let p = null 2) if (callback === undefined) then assign p ... else, p will stay null (that's the only assignation of p) 3) Systematically call p.then(), .catch(), ...

👉 You'll ystematically get the Cannot read property 'then' of null I mentioned when you're using a callback, dont you ?

I didnt look into too much details, but that smells weird to me :)

[oguimbal]

I managed to reproduce it 🙂

// tslint:disable: no-floating-promises
import { Client, Transport } from '@elastic/elasticsearch';

const delay = () => new Promise(res => setTimeout(res, 10));

const cached = new Client({
    nodes: ['http://localhost:9200/'],

    // This mimicks aws-elasticsearch-connector
    Transport: class extends Transport {
        request(params, options = {}, callback) {
            if (typeof options === 'function') {
                callback = options
                options = {}
            }

            if (typeof callback === 'undefined') {
                return delay()
                    .then(() => super.request(params, options));
            }

            // Callback support
            delay()
                .then(() => super.request(params, options, callback));
        }
    }
});

(async () => {
    await cached.search({});
    console.log('End');
})();

👉 The same code works with 7.13.0

[delvedor]

The code above won't work because you are not returning what Transport.request should return. If you want to override the default Transport.request, you should always return return super.request(params, options, callback) at the end.

[oguimbal]

That is arguable, in my opinion.

When using a callback philosophy, you are implicitely asked to call the given callback. But it is not supposed to be mandatory to return something. Else, you are effectively forcing the use of promises, and then I dont see the point of passing a callback as an argument of request().

Anyway, that is how the lib we use (30k weekly downloads) is doing things, and it was working fine until 7.14.0.

Since this was working before, this looked like a regression to me :)

Moreover, the incriminated code I linked looked sufficiently smelly to me to think that something was going on here. Thus, I felt compeled to report this.

That is kind of a sterile debate, but if your lib supports what you consider to be buggy implementations, then I think most people will expect it to keep supporting them in following minor versions.

That said, I'm perfectly happy to be stuck with 7.13.0, my point was only to warn you that this version had potential breaking changes :)

[cesarfd]

Hi, still failing in 7.14.1 and 7.15.0 (using the library from pino-elasticsearch). 7.13.0 works fine.

[rv-vmartyniuk]

Same problem on our end. Version 7.13.0 works perfectly fine.

[lironezra]

There is solution for this? I'm facing same problem in version 7.15.0

Thanks!

[delvedor]

Closed in #1594.