koajs / koa

Expressive middleware for node.js using ES2017 async functions
https://koajs.com
MIT License
35.16k stars 3.23k forks source link

The context can not work with _.cloneDeep #1600

Closed xiedacon closed 2 weeks ago

xiedacon commented 2 years ago

Test codes:

const _ = require('lodash');
const Koa = require('koa');

const app = new Koa();

app.use((ctx) => {
    _.cloneDeep(ctx);

    ctx.body = {
        test: 'test',
    };
});

app.listen(3000);
curl http://127.0.0.1:3000/
  TypeError: Cannot read property 'length' of undefined
      at Object.length (/Users/admin/Downloads/test/node_modules/delegates/index.js:72:24)
      at isArrayLike (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:11401:46)
      at keys (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:13375:14)
      at baseGetAllKeys (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:3094:20)
      at getAllKeys (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:5916:14)
      at baseClone (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:2726:39)
      at /Users/admin/Downloads/test/node_modules/lodash/lodash.js:2733:34
      at arrayEach (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:530:11)
      at baseClone (/Users/admin/Downloads/test/node_modules/lodash/lodash.js:2727:7)
      at /Users/admin/Downloads/test/node_modules/lodash/lodash.js:2733:34

I found it was caused by the code here https://github.com/koajs/koa/blob/19f3353e5da6a0d52fb968a9429523585e42a288/lib/context.js

The simple code is like this:

const _ = require('lodash');

const contextProto = {};
// delegates
Object.defineProperty(contextProto, 'length', {
    get: function () {
        return this.response.length;
    },
    set: function (val) {
        this.response.length = val;
    },
});

// new App()
const app = {
    context: Object.create(contextProto),
};

// new ctx
const ctx = Object.create(app.context);
ctx.app = app;
ctx.response = { length: 1 };

console.log(_.cloneDeep(ctx));
// console.log(_.cloneDeep(app));

When execute _.cloneDeep(ctx), app will also be cloned. But app.context can not be clone, because lodash needs the length proproty to decide whether obj is array-like https://github.com/lodash/lodash/blob/2da024c3b4f9947a48517639de7560457cd4ec6c/isArrayLike.js

It also happens on request and response.

I think maybe we should do some special operations for length property, just like this:

const _ = require('lodash');

const contextProto = {};
// delegates
Object.defineProperty(contextProto, 'length', {
    get: function () {
        return this.response?.length;
    },
    set: function (val) {
        if (this.response) {
            this.response.length = val;
        }
    },
});

// new App()
const app = {
    context: Object.create(contextProto),
};

// new ctx
const ctx = Object.create(app.context);
ctx.app = app;
ctx.response = { length: 1 };

console.log(_.cloneDeep(ctx));
// console.log(_.cloneDeep(app));
dead-horse commented 2 years ago

Is there a specific usage scenario that requires clone ctx?

xiedacon commented 2 years ago

In our case, it happens when we use async_hooks and sequelize at the same time.

At the async_hooks part, we need to use it to pass the ctx, so there is no need to add the ctx parameter to every function.

const { AsyncLocalStorage } = require('async_hooks')

const Koa = require('koa')

const localStorage = new AsyncLocalStorage();
const app = new Koa();

app.use((ctx, next) => {
  localStorage.run({ ctx }, next)
});

app.use((ctx) => {
  ctx.body = fn1()
});

const fn1 = () => fn2()
const fn2 = () => fn3()
const fn3 = () => {
  const { ctx } = localStorage.getStore()

  // do something

  return ctx.query
}

app.listen(3000);

At the sequelize part, we need to use the dialectModule options to pass our own mysql driver to sequelize.

There are two important things:

  1. There is an timer on the mysql driver.
  2. The sequelize will use _.cloneDeep to copy options before querying sql. https://github.com/sequelize/sequelize/blob/main/lib/dialects/abstract/connection-manager.js#L21
const { Sequelize } = require('sequelize')

const dialectModule = {
  intervalId: setInterval(() => {
    console.log('some thing')
  }, 1000)
}

const sequelize = new Sequelize({
  dialectModule
})

// It will call _.cloneDeep() to dialectModule.
sequelize.query('SELECT 1 = 1')

In the end, our code looks like the following:

const { AsyncLocalStorage } = require('async_hooks')

const Koa = require('koa')
const { Sequelize } = require('sequelize')

const localStorage = new AsyncLocalStorage();
const app = new Koa();

app.use((ctx, next) => {
  localStorage.run({ ctx }, next)
});

app.use(async (ctx) => {
  ctx.body = await someController()
});

const someController = async () => someService()
const someService = async () => someModel()
const someModel = async () => {
  const { ctx } = localStorage.getStore()
  const sequelize = new Sequelize({
    dialectModule: {
      intervalId: setInterval(() => {
        console.log('some thing')
      }, 1000)
    }
  })

  console.log(ctx.query)

  // TypeError: Cannot read property 'length' of undefined
  return await sequelize.query('SELECT 1 = 1')
}

app.listen(3000);

It will be thrown when querying sql, even if the async_hooks has no business with it.

The reason is

  1. When using AsyncLocalStorage, it will store the ctx in the Symbol(kResourceStore) property of each timer.
  2. When querying sql, the sequelize will use _.cloneDeep to copy options.
  3. Copying options means copying dialectModule, means copying timer, means copying ctx, and finally equals _.cloneDeep(ctx).
xiedacon commented 2 years ago

An example without sequelize.

const { AsyncLocalStorage } = require('async_hooks')

const _ = require('lodash')
const Koa = require('koa')

const localStorage = new AsyncLocalStorage();
const app = new Koa();

app.use((ctx, next) => {
  localStorage.run({ ctx }, next)
});

app.use(async (ctx) => {
  ctx.body = await someController()
});

const someController = async () => someService()
const someService = async () => someModel()
const someModel = async () => {
  // TypeError: Cannot read property 'length' of undefined
  _.cloneDeep(setTimeout(() => {}, 1000))

  return { some: 'thing' }
}

app.listen(3000);
siakc commented 9 months ago

The reason for this error is that length is a delegated property of ctx. So when lodash tries to read it to see if the object has a length, delegate getter is actually called and I guess as the properties are not fully cloned it fails.

I guess this is fixed in newer version of lodash as it checks to see if the property is not a function first.

jonathanong commented 1 month ago

why do you need to clone the ctx? can't you just pass the ctx itself?