Performance issue with preinstall and postinstall scripts

[vkrol]

Do you want to request a feature or report a bug?
Bug.

What is the current behavior?

If the current behavior is a bug, please provide the steps to reproduce.
Create the following package.json:

{
  "name": "yarn-playground",
  "private": true,
  "scripts": {
    "preinstall": "echo 1",
    "postinstall": "echo 1"
  }
}

Run Measure-Command { yarn } in PowerShell:

TotalSeconds      : 5,4693932

If the package.json does not contain preinstall OR postinstall script then the duration will be:

TotalSeconds      : 0,471668

What is the expected behavior?
I do not understand why the yarn takes so much time if both scripts are present.

Please mention your node.js, yarn and operating system version.
Node.js 8.5.0
Yarn 1.1.0
OS Windows 10

[vkrol]

I think that there is the problem with this timeout

yarn/src/util/blocking-queue.js

Line 47 in 25890c8

this.stuckTimer = setTimeout(this.stuckTick, 5000);

[BYK]

@vkrol great find! Would you like to submit a fix? I'd do the following two:

1. Add this.stucktimer.unref() right after that line so it wouldn't block exiting.
2. Add clearTimeout(this.stuckTimer) right after

   yarn/src/util/blocking-queue.js

   Line 81 in 25890c8

   this.runningCount--;

What do you think? Also /cc @arcanis

[vkrol]

@BYK thanks! I will create the fix. Do I need to create some test for it?

[vkrol]

@BYK I found some confusing behavior in Jest. The fake Timer implementation does not have unref method in Node environment so blocking-queue.js test will fail with an error:

this.stuckTimer.unref is not a function

I asked a question about this in Discord chat.

test("fake Time implementation does not have unref method", () => {
  const realTimer = setTimeout(() => {});
  expect(typeof realTimer.unref).toEqual("function");

  jest.useFakeTimers();

  const fakeTimer = setTimeout(() => {});
  expect(typeof fakeTimer.unref).toEqual("undefined");
});

[BYK]

@vkrol sorry, I couldn't find your message on Discord :( Ping me directly there?

I think instead of using fake timers you can make sure the timer gets released in tests then? unref is node-specific and not really documented anywhere I guess so no surprise Jest doesn't support it. We can summon @cpojer to see if he has any idea tho?

[cpojer]

Yeah, Jest doesn't support it at this time. You'll need to guard it for Jest, like fakeTimer.unref && fakeTimer.unref() to make it work in the test for now.

[vkrol]

@BYK

sorry, I couldn't find your message on Discord :(

Sorry for the confusion: I am asked in Jest Discord chat.

unref is node-specific and not really documented anywhere I guess so no surprise Jest doesn't support it.

Unref is documented in Node.js documentation https://nodejs.org/api/timers.html#timers_timeout_unref

We can summon @cpojer to see if he has any idea tho?

I created the issue in Jest repository jestjs/jest#4559

[vkrol]

@BYK

I think instead of using fake timers you can make sure the timer gets released in tests then?

Fake timers already used there

yarn/__tests__/util/blocking-queue.js

Line 6 in 08411a0

jest.useFakeTimers();

[BYK]

Unref is documented in Node.js documentation https://nodejs.org/api/timers.html#timers_timeout_unref

TIL. Sorry :)

Fake timers already used there

Then how about we go with @cpojer's suggestion and checking for it?

[vkrol]

@BYK do you think that we need to add the following line of code to the BlockingQueue implementation?

this.stuckTimer && this.stuckTimer.unref()

I think that it is somewhat hacky, but if you approve this, then ok :)

[BYK]

It should be this.stuckTimer.unref && this.stuckTimer.unref(). With a comment that points to the Jest issue, I think it's fine.

[vkrol]

It should be this.stuckTimer.unref && this.stuckTimer.unref()

Yes, sure. Sorry.

With a comment that points to the Jest issue, I think it's fine.

Thanks. I will do it.