context is lost on 'end' event of http request stream

[aidinrs]

Here is a sample snippet:

const http = require('http');
var createNamespace = require('cls-hooked').createNamespace;
var session = createNamespace('session-ns');


const server = http.createServer(function (request, response) {
    session.run(function (ctx) {
        session.set('key', 1)
        read(request, response, function (req, res) {
            process._rawDebug(`DONE - key: ${session.get('key')}`)
            res.end(`key: ${session.get('key')}\r\n`)
        })
    })
})

server.listen(8080)

function read(req, res, done) {
    let body = [];
    req
        .on('data', function (chunk) {
            body.push(chunk)
        })
        .on('end', function () {
            body = Buffer.concat(body).toString()
            process._rawDebug(`End - key: ${session.get('key')}`)
            done(req, res)
        });
}

As you can see I have created a http server and read the body by a stream. run the snippet then use curl.
curl http://localhost:8080
console output:

End - key: 1  
DONE - key: 1

for simple GET requests the context is correct, but when you send data in the body the context is lost.
curl http://localhost:8080 -X POST -d aaaaaaaaaa
console output:

End - key: undefined   
DONE - key: undefined

This issues is also related to this skonves/express-http-context#4. The problem is not the body-parser package, I have tracked it down and found that it is caused by the same issue as here, the callback of 'end' event looses context.

Node runtime: reproducible on all v8.14.0, v10.4.0, v10.3.0, and v10.14.1
OS: Ubuntu 18.04
cls-hooked: 4.2.2

[aidinrs]

@Jeff-Lewis I think this issue is related to cls-hooked, I can pass context in the sample by a naive implementation using async_hooks

[Strate]

@AiDirex did you find solution for your problem?

[PoslavskySV]

Same here. Any solution?

[aidinrs]

@Strate @PoslavskySV I decided to use my own naive implementation.

const asyncHooks = require('async_hooks')

let contexts = {}

asyncHooks.createHook({
  init (asyncId, type, triggerAsyncId, resource) {
    if (contexts[triggerAsyncId] !== undefined) {
      contexts[asyncId] = contexts[triggerAsyncId]
    } else {
      contexts[asyncId] = {}
    }
  },

  destroy (asyncId) {
    delete contexts[asyncId]
  }
}).enable()


function run (callback) {
  let eid = asyncHooks.executionAsyncId()
  contexts[eid] = {}
  callback()
}

function set (k, v) {
  let eid = asyncHooks.executionAsyncId()
  let ctx = contexts[eid]
  if (ctx !== undefined) {
    ctx[k] = v
  }
}

function get (k) {
  let eid = asyncHooks.executionAsyncId()
  let ctx = contexts[eid]
  return ctx !== undefined ? ctx[k] : undefined
}

module.exports = {get, set, run, contexts}

It doesn't have the above issue, and I use it with async/await on node 8.14 without any problems. Also, I use these packages on my project: express, body-parse, Sequelize, oauth2-server, cookie-parser, and request-promise-native.
Though I had some issues with nested Promises at first but when I rewrote them with async/await it worked fine.

[PoslavskySV]

@AiDirex Thanks!

[Strate]

@AiDirex have you tried namespace.bindEmitter(), as described in readme of this lib?

[aidinrs]

@Strate no.

[PoslavskySV]

@Strate I tried, still bug is present.

[aidinrs]

@Strate my code snippet worked without any problems so far.

[aidinrs]

This is also a middleware for express which does something like the express-http-context package.

const uuid = require('uuid/v4')
const {run, set} = require('./context')


module.exports = (req, res, next) => {
  run(() => {
      let ctxId = uuid()
      set('context-id', ctxId)
      next()
  })
}

Then in the rest of your middleware chain you can use get('context-id') to retrieve the id for that context.

[Strate]

@AiDirex sounds very promising! I am bit confused, that originally this library has a bit more code, especially inside async hook.

I've tested your code against bluebird, context losed (I think this is bluebird's problem).

Would you release your code as a package with cls-interfaces support? (for example, to just drop-replace this library?)

[aidinrs]

@Strate it may have some issues with non-native promise implementations. I use it with async/await and native promises. It has more code because you can have multiple namespaces, my snippet is just one namespace. However, adding namespaces is easy.
Yes, I plan to publish a package.

[Jeff-Lewis]

@AiDirex I think you might have found the issue/cause for why the http/net tests are failing in the dev branch of cls-hooked. I haven't had a chance to look into it but now that you have a working example, I will compare and see what the difference is. I will publish a new version when it's resolved. Thank for your help!