-
Notifications
You must be signed in to change notification settings - Fork 124
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
Conversation
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
socket 还是会不存在吧
There was a problem hiding this comment.
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 对象
@hyj1991 加一个可以触发 bug 的测试用例? |
这个其实就是 node-v14.x 下的一个更新: http: detach socket from IncomingMessage on keep-alive,会把 res.socket 去掉,这个 case 在 14.x 才会复现,这种要怎么增加 test case 呢? |
增加一个 node 14 的 CI,测试用例指定如果 node 版本小于 14 就跳过 |
@hyj1991 我来加这个测试用例吧。 |
@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 里面包含 |
2.35.0 |
sendMessage('http://120.27.24.200:3000/'); |
嗯,我这里是用 egg 搭建了一个服务器,本来想用 npm registry 测试的,但是它的响应 header 里没有 本地测试主要是得让服务器的响应 header 里带上:
即可。 Reference: #341 |
In node-v14.x,
res.socket
has been detached from IncomingMessage.Reference: nodejs/node#32153
Reference: V14 Semver-Major Commits