iuioiua / r2d2

Minimal Redis Client for Deno.
https://deno.land/x/r2d2
MIT License
56 stars 0 forks source link

Race conditions on sendCommand. #146

Closed tmikaeld closed 1 year ago

tmikaeld commented 1 year ago

Was confused why connections would stall/hang when reading many keys async.

It's because when using 1 connection, redis needs a queue.

As you can see in the older driver here: https://github.com/search?q=repo%3Adenodrivers%2Fredis%20queue&type=code

Something like this maybe (untested):

// Command queue
const commandQueue: Command[] = [];
let isProcessingQueue = false;

// Function to process command queue
async function processQueue(redisConn: Deno.Conn) {
  if (isProcessingQueue) return;
  isProcessingQueue = true;

  while (commandQueue.length > 0) {
    const command = commandQueue.shift();
    if (command) {
      try {
        await writeCommand(redisConn, command);
        const reply = await readReply(readDelim(redisConn, CRLF_RAW));
        console.log(reply); // handle reply as needed
      } catch (error) {
        console.error(error); // handle error as needed
      }
    }
  }

  isProcessingQueue = false;
}

// Updated sendCommand to use command queue
export async function sendCommand(
  redisConn: Deno.Conn,
  command: Command,
  raw = false,
): Promise<void> {
  commandQueue.push(command);
  processQueue(redisConn);
}
tmikaeld commented 1 year ago

Any update on this? I'm also unable to reconnect after a broken pipe, I tried to patch it but whatever I try like Deno.close(conn) or conn.close() but it just stalls forever.

tmikaeld commented 1 year ago

The error I was getting before stalls:

An error occurred: BadResource: Bad resource ID
    at write (ext:deno_net/01_net.js:31:21)
    at TcpConn.write (ext:deno_net/01_net.js:94:12)
    at writeAll (https://deno.land/std@0.200.0/streams/write_all.ts:32:25)
    at writeCommand (https://deno.land/x/r2d2@v1.1.11/mod.ts:81:9)
    at sendCommand (https://deno.land/x/r2d2@v1.1.11/mod.ts:317:9)
    at CommandQueue.processQueue (file:///home/main/workspace/proxy/src/lib/redis.js:27:30)
    at eventLoopTick (ext:core/01_core.js:183:11) {
  name: "BadResource"
}
iuioiua commented 1 year ago

Hi @tmikaeld, I'm very sorry for the late reply. I'll look into this within the next few days.

tmikaeld commented 1 year ago

There's no rush to look at this, we're switching back to denodrivers/redis meanwhile, since we need stable redis in prod.

iuioiua commented 1 year ago

I fixed the race condition issue on my local machine using a simple queue mechanism. Please check and try it out here: https://github.com/iuioiua/r2d2/tree/queue-trial

I'll have to make some fairly fundamental changes to this module because of this, along with some updates relating to deprecations in the Standard Library. Keep your eyes peeled 👀 and let me know how testing of that tree goes 👍🏾

tmikaeld commented 1 year ago

Thanks for looking into this, looks great and the tests pass as well!

Sorry for the late reply, I've been busy with our launch.