-
Notifications
You must be signed in to change notification settings - Fork 13
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
distinguish between read timeout and connect timeout #7
Conversation
lib/index.js
Outdated
exports.request = function (url, opts) { | ||
// request(url) | ||
opts || (opts = {}); | ||
|
||
const parsed = typeof url === 'string' ? parse(url) : url; | ||
|
||
const timeout = opts.timeout || TIMEOUT; | ||
let readTimeout, connectTimeout; | ||
if(isNumber(opts.readTimeout) || isNumber(opts.connectTimeout)) { |
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.
空格。
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.
done
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.
lint 规则竟然没提示。
99dce08
to
591cebf
Compare
修改后,将 read timer 设置为 response/request 的底层依赖的 socket 的属性达到跨调用请求生命周期内的共享(官方暴露的 API,应该比较稳定: request.socket, response.socket) |
socket[READ_TIME_OUT] = readTimeout; | ||
var err = new Error(); | ||
var message = `Timeout(${readTimeout})`; | ||
abort(append(err, 'RequestTimeout', message)); |
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.
这个 code 应该叫 ReadTimeout 了吧。
我感觉对 timer 的清理不够完整。 |
lib/index.js
Outdated
}); | ||
}; | ||
|
||
exports.read = function (response, encoding) { | ||
if (response.socket[READ_TIME_OUT] !== false) { | ||
throw new Error(`RequestTimeoutReadTimeout: ${response.socket[READ_TIME_OUT]}`); |
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.
return Promise.reject(new Error(RequestTimeoutReadTimeout: ${response.socket[READ_TIME_OUT]}
));
更合适。
现在的实现里,在 request 阶段 [connect, onresponse] 能顺利触发 request 的异常抛出。 但开始 read 到 end 之间,出现超时了,不能抛出异常。 |
@JacksonTian 这个库如果要真正设计 read timeout 其实有一个问题: 如果用户调用玩 request 方法后一直不去调用 read 方法,那么 request 方法里面 connect 结束后设置的 read timer 就无法去触发,因为 request 本身返回的是 promise.resolve(response),而且当 request 方法正确返回时,这个 promise 的状态已经是 resolved 了,这时如果 read timer 超时,那里面的 reject 首先没有用,如果改为 throw 强制触发也违背了 promise 状态不能更改的设计原则 |
b7eef2d
to
7167e87
Compare
修改了下逻辑,补了下 read 动作的超时测试,目前只有一种逻辑下无法捕获 read 超时: async function test() {
const res = await httpx.request('/', { readTimeout: 300 });
// 拿到 res 后永远不去读取,此时因为 httpx.request 返回的这个 promise 状态已经是 resolved 了,不会再发生更改 ,导致超时错误没有办法被捕获
// 相反,如果故意等到 300ms 后再去执行 httpx.read 或者在执行 httpx.read 时发生 read timer 超时,是可以正确抛出的
} |
这个是预期的。 |
No description provided.