Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: res.socket is null in node-v14.x #339

Merged
merged 1 commit into from
May 15, 2020
Merged

Conversation

hyj1991
Copy link
Member

@hyj1991 hyj1991 commented May 14, 2020

In node-v14.x, res.socket has been detached from IncomingMessage.

Reference: nodejs/node#32153
Reference: V14 Semver-Major Commits

@atian25 atian25 requested review from fengmk2 and dead-horse May 14, 2020 06:00
res.socket has been detached from IncomingMessage in node-v14.x

Reference: nodejs/node#32153
@@ -592,10 +593,11 @@ function requestWithCallback(url, args, callback) {
if (serverSocketTimeout < freeSocketTimeout) {
// https://github.com/node-modules/agentkeepalive/blob/master/lib/agent.js#L127
// agentkeepalive@4
var socket = res.socket || (req && req.socket);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

socket 还是会不存在吧

Copy link
Member Author

@hyj1991 hyj1991 May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

走到了 onResponse 逻辑的话,req 应该已经赋值了,req.socket 是存在的吧。而且实际上在 v14.x 之前,req.socket === res.socket,这两个是一样的指向原始的 socket 对象

@fengmk2
Copy link
Member

fengmk2 commented May 14, 2020

@hyj1991 加一个可以触发 bug 的测试用例?

@hyj1991
Copy link
Member Author

hyj1991 commented May 15, 2020

@hyj1991 加一个可以触发 bug 的测试用例?

这个其实就是 node-v14.x 下的一个更新: http: detach socket from IncomingMessage on keep-alive,会把 res.socket 去掉,这个 case 在 14.x 才会复现,这种要怎么增加 test case 呢?

@dead-horse
Copy link
Member

这个其实就是 node-v14.x 下的一个更新: http: detach socket from IncomingMessage on keep-alive,会把 res.socket 去掉,这个 case 在 14.x 才会复现,这种要怎么增加 test case 呢?

增加一个 node 14 的 CI,测试用例指定如果 node 版本小于 14 就跳过

@fengmk2 fengmk2 merged commit 175ad2b into node-modules:master May 15, 2020
@fengmk2
Copy link
Member

fengmk2 commented May 15, 2020

@hyj1991 我来加这个测试用例吧。

@hyj1991
Copy link
Member Author

hyj1991 commented May 15, 2020

@fengmk2 一个可以复现的例子:

'use strict';

const urllib = require('urllib');
const Agent = require('agentkeepalive');

const keepaliveAgent = new Agent({
  maxSockets: 1000,
  maxFreeSockets: 100,
  timeout: 1200000,
  freeSocketTimeout: 60000,
});

async function sendMessage(url, data, method = 'GET') {
  const res = await urllib.request(url, {
    method,
    data,
    nestedQuerystring: true,
    timeout: 60000,
    agent: keepaliveAgent,
    contentType: 'json',
  });

  console.log(res.data.toString());
};

sendMessage('http://120.27.24.200:3000/');

这里需要测试 url 对应的服务器返回的 headers 里面包含 connection: keep-alivekeep-alive: timeout=xxx,这个例子在 v14 下面就会走到抛错的逻辑。

@fengmk2
Copy link
Member

fengmk2 commented May 15, 2020

2.35.0

@fengmk2
Copy link
Member

fengmk2 commented May 15, 2020

```js
sendMessage('http://120.27.24.200:3000/');

sendMessage('http://120.27.24.200:3000/');
这个实现得看看

@hyj1991
Copy link
Member Author

hyj1991 commented May 15, 2020

```js
sendMessage('http://120.27.24.200:3000/');

sendMessage('http://120.27.24.200:3000/');
这个实现得看看

嗯,我这里是用 egg 搭建了一个服务器,本来想用 npm registry 测试的,但是它的响应 header 里没有 keep-alive: timeout=xxx 属性,导致走不到这里抛错逻辑。

本地测试主要是得让服务器的响应 header 里带上:

  • connection: keep-alive
  • keep-alive: timeout=xxx

即可。

Reference: #341

fengmk2 added a commit that referenced this pull request Jan 16, 2021
fengmk2 added a commit that referenced this pull request Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants