Fix queue details not loading - because of switch from node-redis to ioredis

[oyvindneu]

I see you already have a PR for this #219 fixing issue #218 - but now you have another option.

Waiting for ioredis to implement this feature to fix this problem seems futile.

Looking at their code (below) it is not very advanced parsing only good enough to use the result when doing readyCheck.

In order for them to expose this API they they need to write new robust code and it seems like this is not prioritized and it is a bigger task than one off parsing for use in Arena. The risk of doing this in Arena is smaller since - since Arena use a list of allowed fields anyway, so any parsing errors will be hidden from the en user.

https://github.com/luin/ioredis/blob/master/lib/redis/index.ts#L558

    const info: { [key: string]: any } = {};

    const lines = res.split("\r\n");
    for (let i = 0; i < lines.length; ++i) {
      const [fieldName, ...fieldValueParts] = lines[i].split(":");
      const fieldValue = fieldValueParts.join(":");
      if (fieldValue) {
        info[fieldName] = fieldValue;
      }
    }

[bradvogel]

Do you have an example that uses this feature?

[oyvindneu]

Sorry for the late reply.

It fixes the regression with Redis statistics not being loaded after bullmq switched to ioredis.

At the moment it looks like this - note the "Could not retrieve value" - this is because arena can't read the result of client.info() the same way it could before (when bullmq relied on node-redis).

This issue: #218

[oyvindneu]

@bradvogel do you need a better example?

[bradvogel]

@oyvindneu Thanks for the ping and apologies for not responding before. Mind adding a quick unit test for this function?

@Rua-Yuki Mind taking a look at this? It's similar to your PR #219.

[Rua-Yuki]

RE comment 1027944237 from @bradvogel on 02 Feb 2022 13:31 UTC:

@Rua-Yuki Mind taking a look at this? It's similar to your PR #219.

Sure thing!

This PR #459 solves #218 satisfactorily only in cases where enableReadyCheck is guaranteed to be falsy. This is only the case with Bull ≥ 4.0.0 (see OptimalBits/bull#1873 (comment)).

For older versions of Bull, where we've ioredis' ready-check enabled by default, stale data is erratically presented still. Please see my review comments for more information.

This PR does technically solve an issue where queue details aren't presented at all in the absence of a ready check, though this is a different fundamental issue, and only a portion of #218:

Unfortunately such is not the case, and ioredis updates the serverInfo object only after initially entering a ready state, and only provided enableReadyCheck is truthy.

---

RE comment 995975417 from @oyvindneu on 16 Dec 2021 16:26 UTC:

At the moment it looks like this - note the "Could not retrieve value" - this is because arena can't read the result of client.info() the same way it could before (when bullmq relied on node-redis).

Presentation of "Could not retrieve value" in particular is not the consequence of a deficiency in reading the result of client.info() but simply the entire absence of stored server info – almost definitely the result of enableReadyCheck being set falsy. (i.e., result is of no consequence but side-effects are)

This may seem like a pedantic distinction though I feel it is important to note because the described fix is off-target the referenced issue and there may be some confusion here.

[Rua-Yuki]

Could eliminate an indent-level by dropping this. Empty lines are insignificant already with the if (idx > 0) conditional and we're guaranteed to have strings given the definition of String#split(...).

Feels like a premature optimisation (esp. given that the frequency of empty lines in INFO output isn't high enough to warrant a special case each iteration; a performance loss will be seen in practice by checking truthiness each time, not that it matters in this function).

[Rua-Yuki]

(pedantic) splitInfo is overly vague – consider instead something like parseRedisServerInfo

[Rua-Yuki]

Similarly, res is a bit arbitrary and might imply a web response, etc. but I digress.

[Rua-Yuki]

I'd expect client.serverInfo || splitInfo(info) to produce erratic/unreliable behaviour.

For example, given enableReadyCheck: true (the default for ioredis currently) we will have client.serverInfo defined and thus used in this instance. This will result in the same stale data issue which #218 describes.

Of course, given enableReadyCheck: false the client.serverInfo is undefined and thus we resort to splitInfo(info). Only in this case will accurate and fresh data be presented.

It'd be better to parse each time so we've fresh data regardless the configuration of enableReadyCheck.

[Rua-Yuki]

As of Bull v4.0.0 we do in fact have enableReadyCheck: false as a guarantee on ioredis clients. That release is still relatively new however and it'd be preferable to solve the problem for older versions of Bull as well considering it is extremely simple to do so.