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

优化Http 客户端 #58

Closed
wants to merge 2 commits into from
Closed

优化Http 客户端 #58

wants to merge 2 commits into from

Conversation

pavlelee
Copy link
Member

1、优化多个客户端同时存在,curl事件统一循环,充分利用curl
2、promise resolve之后,立马加入curl循环,完全异步化。

比如:2个客户端,4个任务。
【客户端1】任务1需要8秒,
【客户端2】任务2需要4秒,
【客户端3】任务3依赖任务1结果需要2秒,
【客户端3】任务4依赖任务2结果需要5秒。

原来的流程会
运行任务1->运行任务2->multi运行任务3、4。耗时:8+4+5 = 17
优化1:
运行任务1、2->multi运行任务3、4。耗时:8+5 = 13
优化2:
运行任务1、2->任务2先完成,运行任务4->任务1完成,运行任务3。耗时:8+2 = 10

Pavle Lee added 2 commits August 23, 2017 16:13
1、优化多个客户端同时存在,curl事件统一循环,充分利用curl
2、promise resolve之后,立马加入curl循环,完全异步化。
@andot
Copy link
Member

andot commented Aug 28, 2017

我看你把一段try的代码去掉了。把 try 去掉没关系吗

@pavlelee
Copy link
Member Author

pavlelee commented Aug 28, 2017

模式变了之后,如果是在curl_multi_info_read获取到$info之前的异常都无法reject,所以我就想直接向上传递,之后的可以加上try。
因为swoole等内部把curl事件统一在一个loop上,但是这个库我觉得不应该依赖这些,最好的设计是:
客户端new Client(Transporter); 这个Transporter统一处理loop,其实和static效果一样,好处是抽象了一个Transporter,其他库做实现就行了。不知道你更偏向那个?

@andot
Copy link
Member

andot commented Sep 10, 2017

没有 try 的话,一些已经发出的请求在出错之后,就没办法清理掉了。另外,你的 $key 是用 $request + 时间戳,如果 $request 比较大,消耗内存会比较多吧。还有就是在 loop 当中的请求循环开始之后未结束之前,在已完成的部分请求中再次发出调用请求,即在 curl_multi_exec 的执行过程中,执行 curl_multi_add_handle 和 curl_multi_remove_handle 会不会有冲突呢,这个我不是很清楚,我看过的文档和例子中,都是在 curl_multi_exec 之前执行 curl_multi_add_handle 的,没见过在 curl_multi_exec 之后还可以 curl_multi_add_handle 的。

@andot
Copy link
Member

andot commented Sep 10, 2017

我不太倾向于加一个 Transporter 让用户去创建,那会增加用户使用的难度,不理解它的用户更可能会乱用。比如用户可能会误认为在任何情况下共享同一个 Transporter 都会提高效率,于是在不能共享 Transporter 的情况下,仍然对多个客户端共享同一个 Transporter,导致出错而难以查找错误。

@pavlelee
Copy link
Member Author

确实是这样,所以我在我们项目自己实现了一个HttpClient,只适用于我们需求的类。这个pr可以关闭了

@andot andot closed this Sep 11, 2017
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

2 participants