Cannot read property 'resurrectTimeout' of undefined

[penge]

🐛 Bug Report

Happened on CI only. The message was Cannot read property 'resurrectTimeout' of undefined

See logs:

• https://github.com/elastic/elasticsearch-js/pull/1142/checks?check_run_id=578970718
• https://github.com/elastic/elasticsearch-js/pull/1142/checks?check_run_id=578970732

Related to #940

Cannot read property 'resurrectTimeout' of undefined

stack:
  ConnectionPool.resurrect (lib/pool/ConnectionPool.js:101:47)
  ConnectionPool.getConnection (lib/pool/ConnectionPool.js:167:10)
  Transport.getConnection (lib/Transport.js:348:32)
  makeRequest (lib/Transport.js:123:30)
  onResponse (lib/Transport.js:150:13)
  ClientRequest.request.on.err (lib/Connection.js:103:9)

at:
  line: 101
  column: 47
  file: lib/pool/ConnectionPool.js
  function: ConnectionPool.resurrect

Looks like there can be dead with an ID that hasn't the mathing connection. And therefore, connection.resurrectTimeout fails resulting in Cannot read property 'resurrectTimeout' of undefined.

[penge]

Here's a detailed run where we can see what is happening:

2020-04-11T17:35:42.2635027Z     markDead-dead [ 'node1' ]
2020-04-11T17:35:42.2635481Z     markDead-connection [ 'node0', 'node3' ]
2020-04-11T17:35:42.2635715Z     resurrect-dead [ 'node1' ]
2020-04-11T17:35:42.2635956Z     resurrect-connection [ 'node0', 'node3' ]
2020-04-11T17:35:42.2636166Z     resurrect-dead [ 'node1' ]
2020-04-11T17:35:42.2636380Z     resurrect-connection [ 'node0', 'node3' ]
2020-04-11T17:35:42.2636612Z     markDead-dead [ 'node1', 'node2' ]
2020-04-11T17:35:42.2636968Z     markDead-connection [ 'node0', 'node3' ]
2020-04-11T17:35:42.2637233Z     not ok 7 - Cannot read property 'resurrectTimeout' of undefined

More here: https://github.com/elastic/elasticsearch-js/pull/1142/checks?check_run_id=579393215

Why this is happening:

• because trying to sort dead by their connection but there is no connection for that ID => markDead()
• because trying to resurrect dead that has no connection for that ID => resurrect()

[penge]

This is how it looks locally:

test/behavior/sniff.test.js 1> markDead-dead [ 'node1' ]
test/behavior/sniff.test.js 1> markDead-connection [ 'node0', 'node1', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> resurrect-dead [ 'node1' ]
test/behavior/sniff.test.js 1> resurrect-connection [ 'node0', 'node1', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> resurrect-dead [ 'node1' ]
test/behavior/sniff.test.js 1> resurrect-connection [ 'node0', 'node1', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> resurrect-dead [ 'node1' ]
test/behavior/sniff.test.js 1> resurrect-connection [ 'node0', 'node1', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> resurrect-dead [ 'node1' ]
test/behavior/sniff.test.js 1> resurrect-connection [ 'node0', 'node1', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> markDead-dead [ 'node2' ]
test/behavior/sniff.test.js 1> markDead-connection [ 'node0', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> resurrect-dead [ 'node2' ]
test/behavior/sniff.test.js 1> resurrect-connection [ 'node0', 'node2', 'node3' ]
test/behavior/sniff.test.js 1> markDead-dead [ 'http://localhost:9200/' ]
test/behavior/sniff.test.js 1> markDead-connection [ 'http://localhost:9200/', 'http://127.0.0.
1:38625/' ]
test/behavior/sniff.test.js 1> resurrect-dead [ 'http://localhost:9200/' ]
test/behavior/sniff.test.js 1> resurrect-connection [ 'http://localhost:9200/', 'http://127.0.0
.1:38625/' ]

Where it's ok and the connections are there.

[penge]

Here's a fix. I've added it here: #1142

Explanation:
Because resurrect() is resolved in the future (it has a callback), it might happen that during this period update() is called (e.g. after a SNIFF after a node was killed) and the list of connections got updated. Soon after, resurrect() would mark a non-existing connection as dead or alive, which would fail because the connection would no longer be in the list of connections. Even if this woudn't fail let's suppose, later called getConnection() would call resurrect() and for the same reasons - not finding the connection in the list of connections, it would fail as well.
I have added a check if the connection is present before marking it dead or alive. This solved the thing.

@delvedor What do you think?

[delvedor]

resurrect is called synchronously when calling getConnection, as well as the find code.
In the past, the update operation could cause a bug where it left a dirty dead array, but after #1127 this issue should no longer exist.

If it does, please provide an example to reproduce the issue, preferably without using the behavior test, as those do not represent a "real world" situation :)

[penge]

Actually, in resurrect I see something like this:

connection.request({
  method: 'HEAD',
  path: '/',
  timeout: this.pingTimeout
}, (err, response) => {
  // ...
  this.markDead(connection)
  // ...
  this.markAlive(connection)
})

And if meanwhile update() would be called to change the connections and reset dead, still, after request is resolved, it would mark a non-existing connection as dead/alive.

Isn't this the root of the cause?

[delvedor]

Oh, now I've understood what you mean!
Yes, that might happen, I'll work on a test case to reproduce it, thanks!

[delvedor]

I've opened #1159 with the fix to this issue, please take a look! :)

[penge]

Yes, added some comments :)