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

distinguish between read timeout and connect timeout #7

Merged
merged 1 commit into from
Mar 15, 2019

Conversation

hyj1991
Copy link
Contributor

@hyj1991 hyj1991 commented Mar 5, 2019

No description provided.

@coveralls
Copy link

coveralls commented Mar 5, 2019

Coverage Status

Coverage increased (+1.5%) to 82.707% when pulling 003953c on hyj1991:master into 25963d1 on JacksonTian:master.

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)) {
Copy link
Owner

Choose a reason for hiding this comment

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

空格。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lint 规则竟然没提示。

@hyj1991 hyj1991 force-pushed the master branch 3 times, most recently from 99dce08 to 591cebf Compare March 11, 2019 12:26
@hyj1991
Copy link
Contributor Author

hyj1991 commented Mar 11, 2019

修改后,将 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));
Copy link
Owner

Choose a reason for hiding this comment

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

这个 code 应该叫 ReadTimeout 了吧。

@JacksonTian
Copy link
Owner

我感觉对 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]}`);
Copy link
Owner

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]}));

更合适。

@JacksonTian
Copy link
Owner

现在的实现里,在 request 阶段 [connect, onresponse] 能顺利触发 request 的异常抛出。

但开始 read 到 end 之间,出现超时了,不能抛出异常。

@hyj1991
Copy link
Contributor Author

hyj1991 commented Mar 13, 2019

@JacksonTian 这个库如果要真正设计 read timeout 其实有一个问题:

如果用户调用玩 request 方法后一直不去调用 read 方法,那么 request 方法里面 connect 结束后设置的 read timer 就无法去触发,因为 request 本身返回的是 promise.resolve(response),而且当 request 方法正确返回时,这个 promise 的状态已经是 resolved 了,这时如果 read timer 超时,那里面的 reject 首先没有用,如果改为 throw 强制触发也违背了 promise 状态不能更改的设计原则

@hyj1991 hyj1991 force-pushed the master branch 4 times, most recently from b7eef2d to 7167e87 Compare March 13, 2019 06:50
@hyj1991
Copy link
Contributor Author

hyj1991 commented Mar 13, 2019

修改了下逻辑,补了下 read 动作的超时测试,目前只有一种逻辑下无法捕获 read 超时:

async function test() {
  const res = await httpx.request('/', { readTimeout: 300 });
  // 拿到 res 后永远不去读取,此时因为 httpx.request 返回的这个 promise 状态已经是 resolved 了,不会再发生更改 ,导致超时错误没有办法被捕获
  // 相反,如果故意等到 300ms 后再去执行 httpx.read 或者在执行 httpx.read 时发生 read timer 超时,是可以正确抛出的
}

@JacksonTian
Copy link
Owner

修改了下逻辑,补了下 read 动作的超时测试,目前只有一种逻辑下无法捕获 read 超时:

async function test() {
  const res = await httpx.request('/', { readTimeout: 300 });
  // 拿到 res 后永远不去读取,此时因为 httpx.request 返回的这个 promise 状态已经是 resolved 了,不会再发生更改 ,导致超时错误没有办法被捕获
  // 相反,如果故意等到 300ms 后再去执行 httpx.read 或者在执行 httpx.read 时发生 read timer 超时,是可以正确抛出的
}

这个是预期的。

@JacksonTian JacksonTian merged commit 9f4b0dc into JacksonTian:master Mar 15, 2019
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.

None yet

3 participants