Description

This fixes a bug where the library does not handle early server errors such as -DENIED or -ERR max number of clients reached.

There may be a better way to do this, this is just a first stab that seems to work.

Checklist

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

These errors should be handled, but I don't think this is the way to go.. I think that an early error should be the same as an error from the "#initator" function, reset the connection and run the "reconnect strategy". I'll do it in v5, then maybe backport to v4. Will keep you posted.

How would #initiator() know when to stop waiting in case there are no initial commands?

Maybe we should run PING if there is no AUTH or SELECT just to make sure the connection is actually open? need to think about that..

That is an extra command that every user of the library will have to execute. I think the logic if the server responds without any previous command, install a handler on demand is best.

@dtikhonov you are right, that makes more sense.. but, instead of pushing a "fake command" into the queue, the logic can be moved to the onReply function that is passed to the #decoder. Also, when this happens it should destroy the current socket and the "reconnectStrategy" should kick in.

Also, when this happens it should destroy the current socket and the "reconnectStrategy" should kick in.

In case of these errors, the server closes the connection, so we don't need anything extra to kick off the reconnect strategy. Or did you want to pass the callback this error? At first glance, it is not obvious to me how to wire this through.

@dtikhonov in the case of "max clients" the server closes the connection, but what about other errors (like LOADING)? I think it's reasonable to "restart" when the client reaches an "undefined state"..

IIUC, LOADING is a transient error: the connection is not closed in that case.

I looked, LOADING does not close the connection:

    /* Loading DB? Return an error if the command has not the
     * CMD_LOADING flag. */
    if (server.loading && !server.async_loading && is_denyloading_command) {
        rejectCommand(c, shared.loadingerr);
        return C_OK;
    }

    /* During async-loading, block certain commands. */
    if (server.async_loading && is_deny_async_loading_command) {
        rejectCommand(c,shared.loadingerr);
        return C_OK;
    }

I think we do not need to do much, just handle those errors by bubbling them up.

This pull request has been automatically marked as stale due to inactivity.
It will be closed in 30 days if no further activity occurs.