search

fastify / fastify-request-context

Request-scoped storage support, based on Asynchronous Local Storage (with fallback to cls-hooked)
MIT License
155 stars 15 forks source link

Node 16.2.0 with Jest break AsyncLocalStorage #49

Closed kibertoad closed 3 years ago

kibertoad commented 3 years ago

Prerequisites

Fastify version

3.x.x

Node.js version

16.2.0

Operating system

Windows

Operating system version (i.e. 20.04, 11.3, 10)

Linux, Windows, doesn't matter

Description

All tests are failing when being run on Node 16.2.0. Seems like all stored values are lost and are being resolved to undefined.

Considering that tests for https://github.com/kibertoad/asynchronous-local-storage are still passing with 16.2.0, looks like this particular plugin is doing something wrong.

Steps to Reproduce

Execute existing test suite on Node 16.2.0

Expected Behavior

Test suite passes.

kibertoad commented 3 years ago

@puzpuzpuz Maybe you have any ideas what could be wrong? Wasn't https://github.com/nodejs/node/pull/36394 supposed to be a non-breaking change?

mcollina commented 3 years ago

I would recommend opening this on Node.js

mcollina commented 3 years ago

@Qard can you take a look?

Qard commented 3 years ago

I'll try to find some time to look at it soon. Just got vaccinated yesterday so I'm a bit out of commission today. 😷

Qard commented 3 years ago

Looks like probably the same issue as was discovered in DataDog/dd-trace-js#1095, which seems to be only producible when running in Node.js 16.2.0 and jest. I'll try to find some time to copy/paste the tests into vanilla scripts without jest possibly tomorrow to verify.

Qard commented 3 years ago

Got a bit of time to try it before bed and as I thought, it only happens within Jest. So Jest must be doing something different. Not sure what. 🤔

puzpuzpuz commented 3 years ago

@puzpuzpuz Maybe you have any ideas what could be wrong? Wasn't nodejs/node#36394 supposed to be a non-breaking change?

Yes, it supposed to be a non-breaking change. And I can also see test failures in https://github.com/puzpuzpuz/cls-rtracer + node v16.2.0 which uses jest.

mcollina commented 3 years ago

The short term fix is to migrate off Jest. It's a good thing to do anyway.

(Jest does an absurd amount of monkeypatching of core, so any internal change is likely to break them).

kibertoad commented 3 years ago

@mcollina Yes, I'm thinking about rewriting tests in TAP. The only concern that I have is that Jest is a very popular framework, and if fastify-request-context will start failing for people using it again in the future, and we don't have a quick way to repro it, that will be inconvenient. So I wonder if we should have also tests in Jest, that will be ignored for now.

kibertoad commented 3 years ago

@puzpuzpuz Gotcha, thank you for the clarification!

targos commented 3 years ago

I posted a reproduction of the issue that doesn't use Jest here: https://github.com/nodejs/node/pull/38577#issuecomment-846536583

blimmer commented 3 years ago

It looks like a fix should land in Node 16.3.0.

kibertoad commented 3 years ago

Confirmed, 16.3.0 resolves the issue.