Revert https://github.com/nodejs/node/pull/20555

[Trott]

Bisecting has revealed that one of the two commits in #20555 is what makes the difference between a CI where async-hooks/test-zlib.zlib-binding.deflate fails and one where it passes. This reverts both commits in that PR because leaving the other commit would just leave unused code.

CI showing the test failing with 23a56e0: https://ci.nodejs.org/job/node-test-commit-linux/19021/

CI showing the test passing with the immediately previous commit 6f6f7f7: https://ci.nodejs.org/job/node-test-commit-linux/19023/

Ping @addaleax @apapirovski

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes

[Trott]

Fixes #20907 but didn't put that in the metadata because I believe we leave revert commit messages pristine. At least, I believe I always have in the past.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/15069/

[Trott]

Since unbreaking CI is one of our primary use cases for fast-track, I'm proposing it here. Please 👍 this comment if you support fast-tracking this PR.

[richardlau]

Rubber stamp LGTM to unbreak the CI.

[apapirovski]

Personally, I'm -1 on reverting. We barely touch C++ code in that PR so it most likely just changed some timing to reveal an existing issue.

Could we instead label the test flaky and try to run valgrind like addaleax requested? Otherwise it will get really difficult to debug this given that it doesn't fail locally or on my own instance of ubuntu1604x64.

[cjihrig]

Rubber stamp LGTM to fix CI, but if a proper fix can be found first, that's great too.

[apapirovski]

Please give me until the morning to continue investigating. This PR was really hard to get landed and it has very little to do with why that test is failing now. I'm making decent progress so I'm hopeful of having an actual fix soon.

[Trott]

Please give me until the morning to continue investigating.

@apapirovski With your -1 on this, this can't land without a TSC vote anyway. So you definitely have at least that long, and quite a bit longer likely if you want/need it.

[Trott]

(By the way, I'd recommend making your -1 explicit with the Request Changes option. Otherwise, someone might miss your objection and land this.)

[Trott]

Two CI failures: arm-fanned which looks build related, and windows which is known flaky/unreliable right now.

arm-fanned re-run: https://ci.nodejs.org/job/node-test-commit-arm-fanned/1446/

[apapirovski]

Sorry, I'm officially blocking because I now know why the test is broken and this isn't the cause. (This PR changed GC timing and now the destructor in Zlib is failing.)

[Trott]

Sorry, I'm officially blocking because I now know why the test is broken and this isn't the cause. (This PR changed GC timing and now the destructor in Zlib is failing.)

👍 Thanks for all the work figuring this all out!

[BridgeAR]

Closing as the test is marked flaky instead.