New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to make this cls work with async function #98

Open
foreverpw opened this Issue Jan 21, 2017 · 22 comments

Comments

Projects
None yet
@foreverpw

foreverpw commented Jan 21, 2017

I have below test code, executed in nodejs 7.4.0 with --harmony-async-await argument.

function newP(id){
    Promise.resolve().then((data)=>{
        ns.run(function(){
            ns.set('cid',id)
            afn()
        })
    })
}
async function afn(){
    var a1 = await log(1)
    var a1 = await log(2)
}
function log(index){
    return new Promise(function (resolve, reject){
         console.log('cid:'+ns.get('cid')+',index:'+index)
         resolve()
    })
}
newP(1);
newP(2);

and the output is:

cid:1,index:1
cid:2,index:1
cid:undefined,index:2
cid:undefined,index:2

what I think it should be is:

cid:1,index:1
cid:2,index:1
cid:1,index:2
cid:2,index:2

It seems that the context is lost, but when I changed the async function to Generator and ran with co, it works well.

function* gen(){
    var a1 = yield log(1)
    var a2 = yield log(2)
}

function newP(id){
    Promise.resolve().then((data)=>{
        ns.run(function(){
            ns.set('cid',id)
            co(gen);
        })
    })
}

newP(1);
newP(2);

So how can I get it work with async function.

@Qard

This comment has been minimized.

Show comment
Hide comment
@Qard

Qard Jan 23, 2017

Collaborator

It's because V8 handles awaited promises with some internal magic rather than actually calling the then(...) method. The patches to maintain the continuation do not get applied. Unfortunately there's nothing that can be done about this currently. We're waiting on the V8 team to provide hooks to the microtask queue so async_hooks can support promises properly, then CLS will need to be ported at some point from the old AsyncListener API to async_hooks.

Collaborator

Qard commented Jan 23, 2017

It's because V8 handles awaited promises with some internal magic rather than actually calling the then(...) method. The patches to maintain the continuation do not get applied. Unfortunately there's nothing that can be done about this currently. We're waiting on the V8 team to provide hooks to the microtask queue so async_hooks can support promises properly, then CLS will need to be ported at some point from the old AsyncListener API to async_hooks.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jan 27, 2017

V8 promise hooks should ship with V8 5.7 and should be available in Node 8.

ofrobots commented Jan 27, 2017

V8 promise hooks should ship with V8 5.7 and should be available in Node 8.

@Qard

This comment has been minimized.

Show comment
Hide comment
@Qard

Qard Jan 27, 2017

Collaborator

@ofrobots Nice! Thanks for the update. 👍

Collaborator

Qard commented Jan 27, 2017

@ofrobots Nice! Thanks for the update. 👍

@yizhiheng

This comment has been minimized.

Show comment
Hide comment
@yizhiheng

yizhiheng Jun 1, 2017

@ofrobots
tried this with Node8, same result
image

yizhiheng commented Jun 1, 2017

@ofrobots
tried this with Node8, same result
image

@santiagodoldan

This comment has been minimized.

Show comment
Hide comment
@santiagodoldan

santiagodoldan Jul 12, 2017

I'm having the same problem as @yizhiheng, node -v 👉 v8.1.2

santiagodoldan commented Jul 12, 2017

I'm having the same problem as @yizhiheng, node -v 👉 v8.1.2

@Francisc

This comment has been minimized.

Show comment
Hide comment
@Francisc

Francisc Jul 25, 2017

Shouldn't this be logged against V8 as well?

Francisc commented Jul 25, 2017

Shouldn't this be logged against V8 as well?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jul 25, 2017

This library will need to migrate to async hooks + promise hooks in order to support async functions. I am not aware of anyone working on this at the moment.

ofrobots commented Jul 25, 2017

This library will need to migrate to async hooks + promise hooks in order to support async functions. I am not aware of anyone working on this at the moment.

@Jeff-Lewis

This comment has been minimized.

Show comment
Hide comment
@Jeff-Lewis

Jeff-Lewis Jul 25, 2017

See cls-hooked. The plan is to eventually PR this into node-continuation-local-storage when we are comfortable with it.

Jeff-Lewis commented Jul 25, 2017

See cls-hooked. The plan is to eventually PR this into node-continuation-local-storage when we are comfortable with it.

@carlisliu

This comment has been minimized.

Show comment
Hide comment
@carlisliu

carlisliu Aug 9, 2017

var getNamespace = require('continuation-local-storage').getNamespace;
var nameSpaceName = 'sample';
var sampleKey = 'sampleKey';
var createNamespace = require('continuation-local-storage').createNamespace;
createNamespace(nameSpaceName);

var namespace = getNamespace(nameSpaceName);

const mongoose = require('mongoose');
const config = {
    dbIp: '127.0.0.1',
    dbPort: 27017,
    dbUser: 'root',
    dbPass: 'root',
    db: 'order'
};

let uri;
if (config.dbUser == '') {
    uri = 'mongodb://' + config.dbIp + ':' + config.dbPort + '/' + config.db;
} else {
    uri = 'mongodb://' + config.dbUser + ':' + config.dbPass + '@' + config.dbIp + ':' + config.dbPort + '/' + config.db;
}

mongoose.connect(uri);

mongoose.Promise = global.Promise;

const db = mongoose.connection;

db.on('error', function(err) {
    console.log(err);
    setTimeout(() => {
        mongoose.connect(uri);
    }, 1000 * 3)
}).once('open', function() {
    console.log('mongodb connected!');
});

const Schema = mongoose.Schema;
const ObjectId = Schema.ObjectId;

const ActivitySchema = new Schema({
    author: ObjectId,
    title: String,
    body: String,
    create_date: Date
});

var activityS = mongoose.model('activity', ActivitySchema, 'activity');


var _find = activityS.find;
activityS.find = function() {
    console.log('sampleKey:', namespace.get(sampleKey));
    return _find.apply(this, arguments);
}

async function find(ctx, next) {
    namespace.set(sampleKey, 'sample');
    let activity = await activityS.find({}).then(function() {
        console.log('sampleKey_find_then:', namespace.get(sampleKey));
    });
    console.log('sampleKey_find:', namespace.get(sampleKey));
    return activity;
}

namespace.run(function() {
    find();
});

tim 20170809181756

this code didn't work as i expected. Look like the same problem with this issue.

carlisliu commented Aug 9, 2017

var getNamespace = require('continuation-local-storage').getNamespace;
var nameSpaceName = 'sample';
var sampleKey = 'sampleKey';
var createNamespace = require('continuation-local-storage').createNamespace;
createNamespace(nameSpaceName);

var namespace = getNamespace(nameSpaceName);

const mongoose = require('mongoose');
const config = {
    dbIp: '127.0.0.1',
    dbPort: 27017,
    dbUser: 'root',
    dbPass: 'root',
    db: 'order'
};

let uri;
if (config.dbUser == '') {
    uri = 'mongodb://' + config.dbIp + ':' + config.dbPort + '/' + config.db;
} else {
    uri = 'mongodb://' + config.dbUser + ':' + config.dbPass + '@' + config.dbIp + ':' + config.dbPort + '/' + config.db;
}

mongoose.connect(uri);

mongoose.Promise = global.Promise;

const db = mongoose.connection;

db.on('error', function(err) {
    console.log(err);
    setTimeout(() => {
        mongoose.connect(uri);
    }, 1000 * 3)
}).once('open', function() {
    console.log('mongodb connected!');
});

const Schema = mongoose.Schema;
const ObjectId = Schema.ObjectId;

const ActivitySchema = new Schema({
    author: ObjectId,
    title: String,
    body: String,
    create_date: Date
});

var activityS = mongoose.model('activity', ActivitySchema, 'activity');


var _find = activityS.find;
activityS.find = function() {
    console.log('sampleKey:', namespace.get(sampleKey));
    return _find.apply(this, arguments);
}

async function find(ctx, next) {
    namespace.set(sampleKey, 'sample');
    let activity = await activityS.find({}).then(function() {
        console.log('sampleKey_find_then:', namespace.get(sampleKey));
    });
    console.log('sampleKey_find:', namespace.get(sampleKey));
    return activity;
}

namespace.run(function() {
    find();
});

tim 20170809181756

this code didn't work as i expected. Look like the same problem with this issue.

@Jeff-Lewis

This comment has been minimized.

Show comment
Hide comment
@Jeff-Lewis

Jeff-Lewis Aug 19, 2017

FYI - trying this with cls-hooked, I received this:

image

Jeff-Lewis commented Aug 19, 2017

FYI - trying this with cls-hooked, I received this:

image

@joepuzzo

This comment has been minimized.

Show comment
Hide comment
@joepuzzo

joepuzzo Sep 28, 2017

Any updates on this? Was attempting to use this library to keep track of trace information for logging in my GraphQL express app and was disappointed to find that the apollo-server-express uses await and i lose context :(

Nevermind turns out I just needed to include https://github.com/othiym23/cls-middleware

joepuzzo commented Sep 28, 2017

Any updates on this? Was attempting to use this library to keep track of trace information for logging in my GraphQL express app and was disappointed to find that the apollo-server-express uses await and i lose context :(

Nevermind turns out I just needed to include https://github.com/othiym23/cls-middleware

@alexkravets

This comment has been minimized.

Show comment
Hide comment
@alexkravets

alexkravets Nov 10, 2017

Guys, thank you for cls-hooked implementation!

alexkravets commented Nov 10, 2017

Guys, thank you for cls-hooked implementation!

@leodutra

This comment has been minimized.

Show comment
Hide comment
@leodutra

leodutra Nov 26, 2017

@Jeff-Lewis is it ok to use cls-hooked in production with frozen Node.js version?

leodutra commented Nov 26, 2017

@Jeff-Lewis is it ok to use cls-hooked in production with frozen Node.js version?

@jrpedrianes

This comment has been minimized.

Show comment
Hide comment
@jrpedrianes

jrpedrianes Jan 18, 2018

Any updates on this? 🙏

jrpedrianes commented Jan 18, 2018

Any updates on this? 🙏

@vdeturckheim

This comment has been minimized.

Show comment
Hide comment
@vdeturckheim

vdeturckheim commented Jan 18, 2018

@jrpedrianes you can check the discussion here othiym23/async-listener#135

@stormit-vn

This comment has been minimized.

Show comment
Hide comment
@stormit-vn

stormit-vn Mar 20, 2018

Hi all,

I am using sequelize-typescript and facing the same issue that transaction does not working

Here is my sequelize initialization

const ns = clsHooked.createNamespace('sequelize-transaction-namespace');
Sequelize.useCLS(ns);

The below code will not working

return this.sequelize.transaction(async (trx) => {
             await Permit.upsert({
                companyId: 1,
                userId: 1
            });

            throw new Error('Should be rollback');
 });

If we pass the transaction manually to nested call, it will work

return this.sequelize.transaction(async (trx) => {
            await Permit.upsert({
                companyId: 1,
                userId: 1
            }, { transaction: trx });

            throw new Error('Should be rollback');
});

Does anyone know how to resolve this issue

stormit-vn commented Mar 20, 2018

Hi all,

I am using sequelize-typescript and facing the same issue that transaction does not working

Here is my sequelize initialization

const ns = clsHooked.createNamespace('sequelize-transaction-namespace');
Sequelize.useCLS(ns);

The below code will not working

return this.sequelize.transaction(async (trx) => {
             await Permit.upsert({
                companyId: 1,
                userId: 1
            });

            throw new Error('Should be rollback');
 });

If we pass the transaction manually to nested call, it will work

return this.sequelize.transaction(async (trx) => {
            await Permit.upsert({
                companyId: 1,
                userId: 1
            }, { transaction: trx });

            throw new Error('Should be rollback');
});

Does anyone know how to resolve this issue

@rostislavprovodenko

This comment has been minimized.

Show comment
Hide comment
@rostislavprovodenko

rostislavprovodenko Mar 20, 2018

Dude, CLS doesn't work with native async/await. You can do what we did, set the target to es5 so that it uses the polyfill instead of the native async/await

rostislavprovodenko commented Mar 20, 2018

Dude, CLS doesn't work with native async/await. You can do what we did, set the target to es5 so that it uses the polyfill instead of the native async/await

@santiagodoldan

This comment has been minimized.

Show comment
Hide comment
@santiagodoldan

santiagodoldan Mar 20, 2018

@stormit-vn as you can see above, you need to use cls-hooked, I'm using it in production and it works just perfect.

santiagodoldan commented Mar 20, 2018

@stormit-vn as you can see above, you need to use cls-hooked, I'm using it in production and it works just perfect.

@stormit-vn

This comment has been minimized.

Show comment
Hide comment
@stormit-vn

stormit-vn Mar 21, 2018

@rostislavprovodenko I've an issue when setting the target to es6, so this won't work.
RobinBuschmann/sequelize-typescript#138

@santiagodoldan yeah, i also trying cls-hooked as well as native CLS and I think both of them are working with my expectation, but I think the problem is related to the sequelize itself. I am using typescript with native async/await, but sequelize only supports with cls-bluebird.

stormit-vn commented Mar 21, 2018

@rostislavprovodenko I've an issue when setting the target to es6, so this won't work.
RobinBuschmann/sequelize-typescript#138

@santiagodoldan yeah, i also trying cls-hooked as well as native CLS and I think both of them are working with my expectation, but I think the problem is related to the sequelize itself. I am using typescript with native async/await, but sequelize only supports with cls-bluebird.

@santiagodoldan

This comment has been minimized.

Show comment
Hide comment
@santiagodoldan

santiagodoldan Mar 21, 2018

@stormit-vn this is what I did

  import { createNamespace } from "cls-hooked"
  import { Sequelize } from "sequelize-typescript"

  const namespace = createNamespace("sequelize-cls-namespace")

  (Sequelize as any).__proto__.useCLS(namespace)

As you can see there you have to assign the namespace to the original Sequelize object, not the one sequelize-typescript returns.

santiagodoldan commented Mar 21, 2018

@stormit-vn this is what I did

  import { createNamespace } from "cls-hooked"
  import { Sequelize } from "sequelize-typescript"

  const namespace = createNamespace("sequelize-cls-namespace")

  (Sequelize as any).__proto__.useCLS(namespace)

As you can see there you have to assign the namespace to the original Sequelize object, not the one sequelize-typescript returns.

@stormit-vn

This comment has been minimized.

Show comment
Hide comment
@stormit-vn

stormit-vn Mar 22, 2018

@santiagodoldan great. Its working now. Thank you very much!

stormit-vn commented Mar 22, 2018

@santiagodoldan great. Its working now. Thank you very much!

@aprilandjan

This comment has been minimized.

Show comment
Hide comment
@aprilandjan

aprilandjan Jun 5, 2018

@santiagodoldan Works like a charm! Thanks!

aprilandjan commented Jun 5, 2018

@santiagodoldan Works like a charm! Thanks!

@leodutra leodutra referenced this issue Jun 19, 2018

Closed

v4.0 #441

14 of 21 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment