BLPop server timeout versus sync/async connection timeout

[Astaelan]

When you use BLPop with a nonzero server timeout, there is no check against the connection sync/async timeout.
I think the library should check against this and throw an exception if you try to use a non zero timeout that exceeds the sync/async timeout.

The reason for this is it caused me hours of debugging before I realized something detrimental that is happening as a side effect.

Let's consider an easy mistake to make, the same mistake I did. The BLPop notes that you need a longer sync/async timeout than server timeout, but the sync/async timeout is in milliseconds. Meanwhile there is no indication that BLPop server side timeout is actually a double in seconds. So while I had my sync/async timeout at 120000 milliseconds, I set my BLPop to 60000 thinking it was in milliseconds as well. Here's where things get interesting.

I was already expecting redis timeouts as a possibility, so I just handled them gracefully in the same way that a server timeout would happen, and let the connection try again for another event. However, something very curious happens when a sync/async timeout occurs. The multiplexer behind all this appears to treat the connection as no longer valid, but still keeps a reference to it, presumably because the BLPop is still waiting. Then it seems to "reconnect" and make a new set of sockets for normal and pub/sub connections under the hood, this increases the redis client connection count.

The net effect of this is that for a connection that was long lived in my case, every 2 minutes it will hit the connection timeout (unintentionally in my case because the server timeout was much higher than the connection timeout) and it starts a new set of sockets for the connection. Meanwhile the old sockets remain open on redis based on client list details.

In my case, over a 12 hour period I racked up 5000 redis connection that I could not account for, and it seems it's due to reconnections of existing connections after timeouts during blocking operations. I think what happened is despite the connect async/sync timeout being hit, the connection is still alive and the BLPop is still "stuck" until the nearly 17 hour timeout from 60000 seconds. Eventually they may have finally got out of that BLPop and closed the connection, but ultimately this all happened because of a bad assumption about seconds versus milliseconds and the library could check for this condition to ensure you don't have server timeouts that exceed the connection timeouts causing extra sockets to hang out when it reconnects. Simply put it doesn't make sense to have a blocking timeout that exceeds the connection timeouts, which also puts support for zero/infinite timeout into question when piggybacking off the SE redis library.

Or, perhaps there is a way in BLPop to detect that the multiplexer has started new sockets and the old ones are waiting to terminate because the BLPop is holding it open, then just unblock and return null like a server timeout would, because the connection is trying to terminate and BLPop is holding it open.

I am not sure about use cases of zero for BLPop server timeout, but I suspect this would cause that situation even more grief with default async/sync timeout settings and causing more background connections that last until the BLPop is released some other way, which could really rack up a lot of connections quickly and over very long periods if more and more connections keep getting made trying to pop the same key.

For what it's worth, I also believe that again in my case, I completely lost events because FIFO ensured the oldest connections got the event and then I believe they left the BLPop with an exception after receiving the event, so there was no return value to process then they closed and the next in line got the next event, but if connections racked up faster than events came in then it never catches up to the current connection after that and new connections just keep adding up stuck in the long wait for an event.

tl;dr

Make sure blocking operations that pass a non zero server timeout cannot pass a timeout that exceeds the connection sync/async timeout. BLPop should check against connection sync timeout, and BLPopAsync should check against connection async timeout. Allowing blocking timeouts that exceed connection timeouts opens a hole for potentially losing data and incurring excessive redis connection hanging.

Easy reproduction by setting connection timeout to 60 seconds, and then do BLPop with a server timeout of 60000 and watch the connections add up. Data will disappear if you wait a few minutes, then PUSH an event externally to the key, and watch the event won't be received by your old connection BLPop code, and your newest connection won't see it either.

[atakavci]

hi @Astaelan , thank you for sharing the details of your case.

Let me analyze the case from my POV and we will try to improve starting from as is state we have.

This all sounds like the issue is happening due to a combination of two things; nature of the multiplexing connection(design choices) + wrong assumptions.
There is clear statement in Redis docs that highlight the arg is in seconds for BLPOP;
" The timeout argument is interpreted as a double value specifying the maximum number of seconds to block. "

That being said, i totally agree that the lib API itself should explicitly declare and explain clearly (with xml docs+ good naming that exposes the details of argument) that the figure is "in seconds", which seems completely missing and we will add it.

For the other part, the dismissed connection remains alive, i need to spend some time on it. For the time being as StackExchange.Redis itself already suggests that multiplexing nature of the lib is not the best fit for using blocking operations and may cause other command calls queued up. Obviously as we already added blocking commands to the API, we have to accept and embrace the risks around it.

To initiate the discussion around how we can improve; i think we should also consider the case that developer might want to wait for BLPOP indefinitely. First, lets see how we can make that happen.
What do you think?

[Astaelan]

All of this sounds good, I just wanted to bring up the potential for lost time of other developers who might run into this.

I agree that there is the possibility someone wants to wait forever, but this means the Sync/Async connection timeouts need an "infinite" setting as well, and I don't believe the SE config supports that which means there is always SOME sort of timeout implied. My main concern is that the lib throw an exception when you try to set a longer timeout on blocking operations than the connection timeouts.

The bottom line seems to be that if your server timeout exceeds the connection timeout then it can do strange things that you really aren't intending, such as holding connections open and losing data.

[mgravell]

Ultimately, if we want a good implementation of any of the blocking operations, then we need to look at extending the core IO loop to support a leased model, presumably an optional extension of the default multiplexer. Blocking operations are simply incompatible with the SE.Redis core, but this is a discussion we've had many times. Without this: yes, blocking operations are going to cause problems.

I'm going to re-add this as a topic on the SE.Redis roadmap, so that we can keep track of it.

Of course, if we do change SE.Redis to support this, we'd presumably add the methods "over there" too.

[Astaelan]

Unfortunately I had to abandon my work around this, I did eventually get a functional implementation but my situation involves X websocket clients each of which have Y contexts they need to block listening for, and while it was nice to see the ability to block on multiple keys at once, the idea of having to have a connection up for every websocket client with a different set of keys wasn't viable. And having a single or pool of connections didn't work for other reasons, so we're back to unreliable pub/sub deliveries for now.

I didn't read deep into the previous discussions on this, but since you're talking about a leased model, at the risk of repeating something from those previous discussions I felt it still worth mentioning a couple other related inconveniences that may be worth hiding more internally while designing this.

First a dedicated/lazy created background connection that is automatically used only for CLIENT UNBLOCK <id>, without having to worry about creating another for this purpose, hide it by allowing the call through the API but using a second data connection. Also maybe a way to automate keeping that client ID known locally on every connection and updated anytime there is an internal reconnection without having to detect it or do it manually before every unblock call to make this operation more immediate and possibly even fire and forget. I would expect the main client ID, not the hidden background connection ID, but both could be exposed for debugging.

Leasing implies there may be some work around tracking and managing all this, so I hope these might offer some useful insights to my adventure working around it.

[mgravell]

CLIENT ID tracking has been present internally in SE.Redis for quite a while now, so: nothing to do there, and presumably in any lease model we would still retain a separate connection for regular multiplexed usage (outside of any leased API), so to unblock, we'd just use that connection. If anything, I think this is the key problem:

The idea of having to have a connection up for every websocket client with a different set of keys wasn't viable

Although that is perhaps solvable by abstracting the "how" - for example, if there was a ListPopper (naming is hard) API something like:

class ListPopper
   event ValueReceived (tuple: RedisKey, RedisValue)
   method Add(params RedisKey[] keys)
   method Remove(params RedisKey[] keys)

which sat atop a leased connection (or connections: cluster), where Add/Remove issues CLIENT UNBLOCK (via the muxer) and re-issues the BRPOP (or whatever) if needed; so from the consumer side, you don't have a connection-per-websocket; you just have a single ListPopper and let the library worry about the rest.