chore(test): easy way to get cluster stuck

[AVVS]

No description provided.

[AVVS]

Pushed a possible solution to the problem

Problem that I'm trying to solve:

resolve is lost when we issue .disconnect(true)

Workaround

start tracking all .connect() promises via .connectionPromise private variable, which exposes resolve and reject for the next connect promise and self-erases upon resolution

Things to think about

• garbage collection - I do seem to be cleaning up everything, but it still introduces tons of cross-references
• instead listen to events (?), ie ready, close inside the ready handler alongside .disconnect(true)
• simply reject() .connect() promise - that requires a breaking change to current behaviour, as one would have to implement retrying outside of ioredis
• reject & implement retrying (via bluebird-retry library for instance)

[AVVS]

@luin any thoughts on that?

[AVVS]

@shaharmor issue described here - #709

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 7 days if no further activity occurs, but feel free to re-open a closed issue if needed.

[AVVS]

@luin what do you think of the approach, should we just redo this into event-based handling instead of chaining promises?

[luin]

@luin what do you think of the approach, should we just redo this into event-based handling instead of chaining promises?

Sorry for the late response! Been pretty busy lately 😢 . This approach works while add a little complexity. I would consider Cluster#connect() failed when the current connection attempt is not successful (although the subsequent ones may success). This keeps the same behavios as Redis#connect(). People need to rely on the events like "ready"/"connect"/"close"/"end" if they need to know the connection status in a higher level (most cases will be this).

Currently, the problem seems to be that we remove the close handler too early so the "reject()" won't be called if a disconnection happens between the "refresh" event and "ready" event. To solve that, we may move the removeListener('close') to the ready event callback, so the Cluster#connect() will be rejected in your test case (although the cluster wil be ready eventually). What do you think?

[AVVS]

Currently, the problem seems to be that we remove the close handler too early so the "reject()" won't be called if a disconnection happens between the "refresh" event and "ready" event. To solve that, we may move the removeListener('close') to the ready event callback, so the Cluster#connect() will be rejected in your test case (although the cluster wil be ready eventually). What do you think?

I think rejecting makes sense, and then users can handle what to do - as long as promise is not stuck forever we are already way ahead of this

At the same time I think it would be great to add a convenience method, which would do auto-reconnect sort of thing with listening to ready/close/error events and possibly some sort of auto-retry strategy with a finite end

[AVVS]

@luin do you think you'd have time to do something about this? :)

[luin]

@AVVS Not have time digging into it currently, but likely can find some time for it within the next two months. I think the issue still exists, doesn't it?

[AVVS]

@luin the issue still exists, indeed :) do you think you might have time to review the code or come up with a better solution than the one I've coded?