albert-team / rebloom

Minimalistic RedisBloom client for Node.js
https://albert-team.github.io/rebloom
MIT License
19 stars 6 forks source link

Connection timeout. #2

Closed luhhujbb closed 5 years ago

luhhujbb commented 5 years ago

Hi @lqmanh, thanks for your work !

We are trying to use you library, but we are facing to timeout on each attempt to connect using both bloomfilter and cuckoofilter.

Maybe something is wrong on our side but even with a localhost redis and all params to default we are facing this issue.

Many thanks for your help !

lqmanh commented 5 years ago

Hi @luhhujbb,

Rebloom internally uses Red, whose default connection timeout is 3s. You can increase that value by either:

  1. Pass Red options via redisClientOptions:
    const filter = new BloomFilter('somekey', {
    redisClientOptions: { timeout: 10 * 1000 }
    })
  2. Use a custom Red instance:
    
    const Red = require('@albert-team/red')

const redisClient = new Red('127.0.0.1', 6379, { timeout: 10 * 1000 }) const filter = new BloomFilter('somekey', { client: redisClient })


However, you said that you encountered timeout even on localhost, so I doubt increasing timeout will do any help.
If possible, could you give me more details about which version of Redis, RedisBloom and Rebloom you're using?
luhhujbb commented 5 years ago

Hi, You're right increasing timeout doesn't help.

Currently we encountered this weird behaviour with the following environment:

Many thanks !

lqmanh commented 5 years ago

Have you tried out the get started example? Please try it out to check if it works. You can also try to run some RedisBloom commands from redis-cli to make sure the module was loaded into redis-server successfully.

luhhujbb commented 5 years ago

Hi,

I've tried the get started example. And it doens't run either.

We have the following stack trace: (node:23267) UnhandledPromiseRejectionWarning: Error: Timed out after 3000ms at Timeout.setTimeout (/*/node_modules/node-wait-until/index.js:30:16) at ontimeout (timers.js:482:11) at tryOnTimeout (timers.js:317:5) at Timer.listOnTimeout (timers.js:277:5) (node:23267) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1) (node:23267) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

We are making a benchmark with another jslib (redis node package v2.8.0 with commands extension to support bloomfilter and cuckoo filter) and we don't have issue to connect to our redis server and pass commands.

We are using official RedisBloom docker image. And RedisBloom commands runs without issue.

lqmanh commented 5 years ago

Could you do a simple test with @albert-team/red? I need to know exactly whether this bug is associated with Rebloom or Red. You just need to check if this script works:

const Red = require('@albert-team/red')

const main = async () => {
  const client = new Red('127.0.0.1', 6379)

  console.log(client.ready) // false
  await client.connect()
  console.log(client.ready) // true

  console.log(client.disconnected) // false
  await client.disconnect()
  console.log(client.disconnected) // true
}

main()
luhhujbb commented 5 years ago

Hi,

I've test the script you provide me and it fails at:

await client.connect()

with a timeout exception. So it's seems the issue comes from Red.

lqmanh commented 5 years ago

Thank you for helping me! However, I still cannot reproduce this bug. Could you give me a little more info about your OS?

luhhujbb commented 5 years ago

Hi,I've found this on nodejs docs:

https://nodejs.org/api/net.html#net_event_ready

It seems that 'ready' event is broadcasted only on node version > 9.11.

https://github.com/albert-team/red/blob/1d6918dbe871d2a1b90c64ec784b41be7cf949a8/src/index.js#L85

I will try your code with a more recent node version.

lqmanh commented 5 years ago

Thank you so much! I truly appreciate that!

luhhujbb commented 5 years ago

Hi,

I've made a test with node 10.10 and no issue anymore.

So maybe you can update readme to follow node documentation or replace 'ready' event by a 'connect' event to have a better node compatibility.

Anyway, many thanks for your help !

luhhujbb commented 5 years ago

Hi,

I've made a test replacing 'ready' event with a 'connect' one using your simple test code: it has the desired behavior and it runs perfectly on node 8.10.

However node 8 LTS end of life is reached on 31/12/2019, so maybe an update of readme is sufficient.

lqmanh commented 5 years ago

Thank you! However, I believe I should extend Red's support for Node.js 8 as it should from the beginning. I've bump Red version to v0.7.1 and everything should work well now. If nothing else, I'll close this issue now.

luhhujbb commented 5 years ago

Hi, Could you bump the red dependency in rebloom ? Many thanks !

lqmanh commented 5 years ago

No worry, if you reinstall dependencies, it'll fetch the latest bugfix release of Red for you.

I'm working on the docs, so when I have more time, I'll bump Red version with Rebloom v2.0.0.

lqmanh commented 5 years ago

Hi @luhhujbb, I've bumped version of Rebloom to v2.0.0. You can upgrade it by running:

npm up @albert-team/rebloom

Earlier I was wrong because just reinstalling dependencies won't make it fetch Red v0.7.1. I bumped it in package.json in this release, though.

luhhujbb commented 5 years ago

Many Thanks !