10.7.0 broke the Yarn tests

[arcanis]

• Version: 10.7.0
• Platform: OSX
• Subsystem: http.request

The 10.7.0 release is causing the Yarn test suite to fail (which prevents us from releasing the 1.9, but also might cause issues with everyone already using Yarn), something related to timeouts:

TypeError: sock.setTimeout is not a function
    at setSocketTimeout (_http_client.js:739:10)
    at ClientRequest.setTimeout (_http_client.js:725:5)
    at setReqTimeout (/Users/mael/yarn/node_modules/request/request.js:817:16)
    at ClientRequest.<anonymous> (/Users/mael/yarn/node_modules/request/request.js:860:9)
    at ClientRequest.emit (events.js:187:15)
    at tickOnSocket (_http_client.js:645:7)
    at onSocketNT (_http_client.js:684:5)
    at process._tickCallback (internal/process/next_tick.js:63:19)

A single commit has been pushed to this file between the 10.6.0 and the 10.7.0 and it happens to change the timeout behavior, which would be consistent with the reported error: 949e885 (cc @killagu).

The repro: clone [email protected]:yarnpkg/yarn and run:

$> yarn install
$> yarn test-only integration-deduping -t 'install should dedupe dependencies avoiding conflicts 8'

I realize it's not a great repro, I'm still working on finding a shorter one.

[ChALkeR]

Thas commit comes from #21204.

/cc @Trott @nodejs/http

@arcanis a shorter testcase would be helpful.

[ChALkeR]

@arcanis This does not look to be observed when run without the mock from https://github.com/yarnpkg/yarn/blob/master/__tests__/__mocks__/request.js.

[ChALkeR]

A reproducable testcase without all the other yarn stuff (but with request)

const request = require('request');
const https = require('https');
const fs = require('fs');

const opt = {"url":"https://registry.yarnpkg.com/yeoman-environment","method":"GET","headers":{"User-Agent":"yarn/1.10.0-0 npm/? node/v10.7.0 linux x64","Accept":"application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*"},"json":true,"gzip":true,"forever":true,"retryAttempts":0,"strictSSL":true,"cert":"","key":"","timeout":30000};

const req = request(opt, () => {});

req.httpModule = {
  request: function request(options, callback) {
    const loc = 'yeoman-environment.bin';
    options.agent = null;
    options.socketPath = null;
    options.createConnection = () => {
      return fs.createReadStream(loc);
    };
    return https.request(options, callback);
  }
};

This is basically what Yarn does.

[arcanis]

You're right, seems like this is caused in particular by the mock of createConnection:

options.createConnection = (): ReadStream => {
    return fs.createReadStream(loc);
};

The previous code was attaching on the connect event from the fake socket, which was never triggered and so the setTimeout was never called. Now that setTimeout is always called when the socket is there, the bug triggers.

I think this is on Yarn, so I'm going to close this issue. Feel free to reopen if you think it hides a more problematic regression.

[arcanis]

And thanks for your help, greatly appreciated!

[dougwilson]

If it's the mock, my guess is that it is due to

  options.createConnection = (): ReadStream => {
    return fs.createReadStream(loc);
  };

[dougwilson]

Wow a lot of conversation happened while I was posted that :) disregard

[ChALkeR]

@arcanis This is one of the reasons why trying to keep the reproducable testcases minimal matters.

I attempted to construct a testcase without yarn. To trace that, I noticed that the error was coming from the request module (as observed in the original trace), and tried to check how exactly are you using request. That was done by adding these two lines to it:

--- node_modules/request/request.js
***************
*** 93,94 ****
--- 93,96 ----
  function Request (options) {
+   console.log('Request', JSON.stringify(options));
+   console.log((new Error()).stack)
    // if given the method property in options, set property explicitMethod to true

That brought me directly to the mock file (and gave the short testcase in the comment above).

Hope that helps 😉.

[arcanis]

Yeah, I actually logged the options to try to make repros, but completely forgot we mocked request, so didn't check the stack 😐

[Trott]

Related: There's a PR to get yarn into citgm but it stalled. If someone wants to take it over and get it across the finish line, it would be great to have citgm flag these kinds of things for us before a release: nodejs/citgm#560

[targos]

@ChALkeR little debugging tip: instead of the double console.log, you can do console.trace('Request', JSON.stringify(options))