WeightedConnectionPool.getConnection returns null that leads to NoLivingConnectionsError error

[jodevsa]

🐛 Bug Report

requests sent after 6 requests that failed due to a timeout with a weightedConnectionPool that has 2 upstreams will fail with NoLivingConnectionsError even if the upstreams is running again and not timing out anymore.

This is mostly due to the wrong assumption mentioned here:
https://github.com/elastic/elastic-transport-js/blob/main/src/pool/WeightedConnectionPool.ts#L54

if the GCD is 1 and both weights reached 1 after the subtraction we would need about 800 loops for current weight to reach 1 to be able to choose one of the weights

To Reproduce

Steps to reproduce the behavior:
1- clone my branch #54 which has a new test that should reproduce the bug
2- npm run build && node_modules/tap/bin/run.js test/unit/transport.test.ts

Paste your code here:

test('upstreams are down for the first 7 requests', async t => {


  const pool = new WeightedConnectionPool({ Connection: MockConnectionTimeoutForThefirstSevenRequests })

  pool.addConnection('https://localhost:9200')
  pool.addConnection('https://localhost:9201')

  const transport = new Transport({ connectionPool: pool, maxRetries: 0 })
  for(let i=0;i<20;i++){
  try {
    await transport.request({
      method: 'GET',
      path: '/hello'
    })
  } catch (err: any) {
    console.log(err.name)
    t.equal(err.name, 'TimeoutError')
  }
}
})




export class MockConnectionTimeoutForThefirstSevenRequests extends BaseConnection {
  requestCount = -1
  async request (params: ConnectionRequestParams, options: ConnectionRequestOptions): Promise<ConnectionRequestResponse>
  async request (params: ConnectionRequestParams, options: ConnectionRequestOptionsAsStream): Promise<ConnectionRequestResponseAsStream>
  async request (params: ConnectionRequestParams, options: any): Promise<any> {
    
    return new Promise((resolve, reject) => {

      this.requestCount++;
      if( this.requestCount<=6){
      process.nextTick(reject, new TimeoutError('Request timed out'))
      }
  


      const body = JSON.stringify({ hello: 'world' })
      const statusCode = setStatusCode(params.path)
      const headers = {
        'content-type': 'application/json;utf=8',
        date: new Date().toISOString(),
        connection: 'keep-alive',
        'content-length': '17',
        'x-elastic-product': 'Elasticsearch'
      }
      process.nextTick(resolve, { body, statusCode, headers })
    })
  }
}

Expected behavior

we are able to use the pool after the upstreams are up again

Your Environment

• node version: v16.0.0
• master branch of this repo
• os: Mac

[jodevsa]

detected while working on nodejs/undici#1065 and taking inspiration from the logic in this repo

[jodevsa]

I'd like to open a PR with a fix after someone verifies my findings

[rudolf]

Related elastic/elasticsearch-js#1714 (comment)

[jodevsa]

@rudolf can I work on a fix?

[rudolf]

@jodevsa As far as I can tell we've got no one internally working on this so a fix would be very welcome!

//cc @delvedor

[technige]

Hi @jodevsa. We would be delighted to receive a pull request for this issue. Please read the contributing guidelines, and let us know when your fix is ready to review. Thanks :)

[rudolf]

The algorithm seems to be inspired by https://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.863.3985&rep=rep1&type=pdf

[jodevsa]

I just saw this :(

HI @technige ,

Thanks for your kind message. I'll start working on a fix!