fastify / fastify-redis

Plugin to share a common Redis connection across Fastify.
MIT License
198 stars 31 forks source link

Update index.js #156

Closed JustCursed closed 2 years ago

JustCursed commented 2 years ago

add next()

Checklist

JustCursed commented 2 years ago

without it, it doesn't work at all

C:UsersUser ests
ode_modulesastifyastify.js:533
        ? appendStackTrace(err, new AVVIO_ERRORS_MAP[err.code](err.message))
                                ^
FastifyError [Error]: fastify-plugin: Plugin did not start in time: '@fastify/redis'. You may have forgotten to call 'done' function or to resolve a Promise
    at manageErr (C:UsersUser   ests
ode_modulesastifyastify.js:533:33)
    at C:UsersUser  ests
ode_modulesastifyastify.js:520:11
    at Object._encapsulateThreeParam (C:UsersUser   ests
ode_modulesavviooot.js:557:7)
    at Boot.timeoutCall (C:UsersUser    ests
ode_modulesavviooot.js:453:5)
    at Boot.callWithCbOrNextTick (C:UsersUser   ests
ode_modulesavviooot.js:435:19)
    at release (C:UsersUser ests
ode_modulesastqqueue.js:149:16)
    at Object.resume (C:UsersUser   ests
ode_modulesastqqueue.js:82:7)
    at C:UsersUser  ests
ode_modulesavviooot.js:174:18
    at C:UsersUser  ests
ode_modulesavvioplugin.js:275:7
    at done (C:UsersUser    ests
ode_modulesavvioplugin.js:200:5) {
  code: 'FST_ERR_PLUGIN_TIMEOUT',
  statusCode: 500,
  cause: AvvioError [Error]: Plugin did not start in time: '@fastify/redis'. You may have forgotten to call 'done' function or to resolve a Promise
      at Timeout._onTimeout (C:UsersUser    ests
ode_modulesavvioplugin.js:122:19)
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7) {
    code: 'AVV_ERR_READY_TIMEOUT',
    fn: <ref *1> [Function: fastifyRedis] {
      default: [Circular *1],
      '@fastify/redis': [Circular *1],
      [Symbol(skip-override)]: true,
      [Symbol(fastify.display-name)]: '@fastify/redis',
      [Symbol(plugin-meta)]: { fastify: '4.x', name: '@fastify/redis' }
    }
  }
}
mcollina commented 2 years ago

Given this is automatically tested and CI verifies this works, I'm a bit skeptical. Can you please add a unit test for the specific case you are testing against?

climba03003 commented 2 years ago

I would check if the connection of redis allowed before doing a next callback in the way bypass all connection check. Please read https://github.com/fastify/fastify-redis#redis-connection-error

Eomm commented 2 years ago

Please, setup your https://www.fastify.io/docs/latest/Reference/Server/#plugintimeout

Even if you call next and the app starts, is the application connected to redis?

JustCursed commented 2 years ago

sorry for the long replies yes, my application connected to redis i can make a video with it

Uzlopak commented 2 years ago

We need a unit test to understand for which edge case your patch is necessary.

JustCursed commented 2 years ago

sorry, i don't know how to do unit tests

Uzlopak commented 2 years ago

If you provide a minimal viable code example where the bug occurs than we could have a look at it

JustCursed commented 2 years ago

it my code

const { uid } = require('uid');

const fastify = require('fastify')({
    logger: {
        transport: {
            target: 'pino-pretty',
            options: {
                colorize: true,
                singleLine: true,
                translateTime: 'dd-mm-yyyy HH:MM:ss:l',
            }
        }
    },
    trustProxy: true,
    return503OnClosing: true,
    ignoreTrailingSlash: true,
    ignoreDuplicateSlashes: true,
    genReqId: () => uid(16)
});

fastify.register(require('@fastify/redis'), {
    url: 'localhost',
    port: 6379,
});

fastify.get('/', async (req, rep) => {
    await fastify.redis.set("mykey", "value");
    rep.send(`it works!`);
});

fastify.listen({ host: '127.0.0.1', port: 5000 });
JustCursed commented 2 years ago

it works only with next function here

JustCursed commented 2 years ago

you may not accept a pull request i just want to fix my problem and share my edits i fixed it and share my edits i don't care what happens next, i did what i wanted to do ¯\_(ツ)_/¯

JustCursed commented 2 years ago

i figured out exactly what the mistake was the error I threw above only occurs when there is no connection to redis you don't have error handling for that case this makes it hard to understand why you're getting the error

JustCursed commented 2 years ago

but my amendments still solve the problem ¯\_(ツ)_/¯

climba03003 commented 2 years ago

i figured out exactly what the mistake was the error I threw above only occurs when there is no connection to redis

Thanks for the confirmation. It is documented in https://github.com/fastify/fastify-redis#redis-connection-error

climba03003 commented 2 years ago

but my amendments still solve the problem

It is by design do not allow the application to startup without database connection. Application wouldn't works when it use a database and the database is not connected.

User should be aware there are something wrong before application started, not silently ignore it.