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

PSR Request的exec()方法是否应该实现在saber上比较妥当? #35

Open
ihipop opened this issue Apr 9, 2019 · 17 comments
Open
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ihipop
Copy link
Contributor

ihipop commented Apr 9, 2019

PSR的RequestInterface没有规定实现exec方法,所以我设计组装一个http客户端无关的request的时候,我肯定不能绑定和客户端强相关的exec方法到Request上,因为每个客户端的异常类型、处理逻辑都不相同。

我设计一个composer组件,在组装请求部分,返回了个psr对象,本意是guzzle或者saber等支持PSR标准的HTTP客户端都可以按psr标准把这个对象代表的请求发送出去,现在Guzzle可以做到($guzzleClient->send($PSRrequest))而saber因为把PSR相关的处理逻辑绑定到他自定义的Request上,导致这样的设计没法实施。

@ihipop
Copy link
Contributor Author

ihipop commented Apr 9, 2019

另外 现有的PSR实现其实不符合PSR标准。按照PSR message的标准,所有的PSR的Request都应该是只读的,也就是说 $requestNew=$request->withBody(...),
$requestNewwithBody执行后的结果,$request是之前的结果

https://www.php-fig.org/psr/psr-7/

image

@twose
Copy link
Member

twose commented Apr 10, 2019

@ihipop
一个是Swoole圈都不是很喜欢PSR设计里内存拷贝的部分(设计上来说的确很好, 将其视为标量, 但免不开性能的急速下降)
二是Saber库是我从自己以前的爬虫项目中抽出来, 稍微迎合了社区风格(但却没有完全遵循规范)实现的, 我对社区制定这个规范的理解也相当有限
三是Saber是和Swoole强相关的, 比如用的defer这种特性之类的细节, 我不清楚在guzzle里如何做到, 所以没有去考虑做一个guzzle handler而是自己写了一个

你说的实现在Saber上是正确的主意, 我会继续构思重构这一部分

Saber2.0已经在我的计划中了, 如果你有什么想法建议可以继续讨论

@twose twose added enhancement New feature or request good first issue Good for newcomers labels Apr 10, 2019
@ihipop
Copy link
Contributor Author

ihipop commented Apr 11, 2019

我的意思也不是让你去做一个guzzle handler,因为确实很多强Swoole相关的东西。
说一下我实际生产上遇到的问题和需求吧:

  • 我本身做一了个Composer扩展(以下简称扩展),需要依赖HTTPS?客户端做外部交互
  • 为了协程兼容、传统兼容或者更大的灵活性,我肯定不愿意绑定到具体的某个客户端上,包括Guzzle或者简单实现的Curl或者Saber
  • 为了方便的绑定客户端,我在这个扩展内部实现了一个简单的DI容器,用来注入管理所需要的客户端或者依赖,当用于协程环境的的时候,扩展在协程内调用是直接传递新建的DI对象实现的,一个DI实例Application代表了这个扩展的所有可调用子实例或者依赖,从而实现协程间扩展客户端的隔离。通过简单配置 我就可以决定自己到底是用什么客户端来发送请求。
  • 于是我的扩展设计上是把需要发送的请求用PSR7的RequestInterface描述所有请求内容,然后用支持PSR的HTTP客户端发送这个请求,这方面,Guzzle的设计是,他的Request对象本身就是PSR兼容的,这样,当我需要使用Guzzle做客户端的时候,我只要在配置DI里面注入Guuzle客户端实例,只简单写一下Guzzle的发送和异常处理,就可以发送这个Request。
  • 问题到Saber上,就是Saber的Request不是完全PSR兼容的,其发送是Request自己发送。导致我生成的描述请求的PSR Rquest是没法简单的交给Saber发送的,只能自己把请求拆开再组装成Saber自己的Request
  • 希望将来设计的时候,把Request和具体的发送请求拆开。Guzzle曾经有很多批评说过度封装,但是从现在看,有些设计还是很有必要的。现在Guzzle可以很简单的用自己快捷的语法组装发送请求,也可以用PSR的方式发送请求。
  • PSR的RequestInterface是否只读得看你个人设计需要,我倒是觉得,这都小问题,只读变成非只读不影响功能,因为遵守只读规范的话,就算你是可读写的,按照只读的用法也不会出错,反而PSR兼容是要紧一点

@ihipop
Copy link
Contributor Author

ihipop commented Apr 11, 2019

二:客户端重用长连接

现在的文档里面说客户端会自动复用长连接,看了代码确实对同目的地同端口的服务器都做了连接池,但是,实测如果走PSR方式,客户端并不复用长连接,连接不停的出现SYN_SENT、ESTABLISHED、LocalPort的状态切换。
如下是PSR Request转换代码,测试服务器是 http://httpbin.org/anything

image

动图:

深度录屏_选择区域_20190411165555

@twose
Copy link
Member

twose commented Apr 11, 2019

@inipop
是否开启了全局配置'use_pool' => true, 默认是关闭连接池的

@ihipop
Copy link
Contributor Author

ihipop commented Apr 11, 2019

@twose 你是对的,以为和Guzzle一样 单一实例会自动复用长连接,看了你代码你是和池绑定到了一起,只有开启池的时候才会使用长连接,否则就算重复请求同一个服务器也不会使用长连接。
当初是基于什么考虑要和池绑定到一起呢?单一客户端多次请求同一个服务端口也可以长连接啊?

@twose
Copy link
Member

twose commented Apr 11, 2019

设计上池是全局共享的, 开启use_pool的实例会从池中尝试取用连接, 不开启的则自己建立连接

@ihipop
Copy link
Contributor Author

ihipop commented Apr 12, 2019

@twose 但是文档里面 Keepalive默认也是true,并没有说明 use pool以后keepalive才会生效
image
而且我看了代码,用PSR()方法是生成了一个默认客户端,应该来说这个客户端默认就应该支持长连接

@ihipop
Copy link
Contributor Author

ihipop commented Apr 12, 2019

生成的PSR对象打印 var_export($psr->getKeepAlive()); 结果也是true的 但是如果不开启池,就不是长连接 这里应该是有问题

具体的问题代码如下:

saber/src/Request.php

Lines 504 to 516 in 5cd1134

if (!$this->client) {
/** create a new coroutine client */
$client_pool = ClientPool::getInstance();
if ($this->use_pool && $client = $client_pool->getEx($host, $port)) {
$this->client = $client;
} else {
$options = [
'host' => $host,
'port' => $port,
'ssl' => $ssl
];
$this->client = $client_pool->createEx($options, !$this->use_pool);
}

这这判断如果本request不和某个client关联,就检查连接池设置,如果没开启池,就始终创建新连接。这样就算默认的Request设置了keepalive是true,也会导致因为客户端每次都是新建的,不使用长连接。这与文档描述行为:长连接默认开启不符。

@twose
Copy link
Member

twose commented Apr 12, 2019

@ihipop 不开启池 你也没保存客户端对象 对象就析构了 连接自然就断开了

@ihipop
Copy link
Contributor Author

ihipop commented Apr 12, 2019

@ihipop 不开启池 你也没保存客户端对象 对象就析构了 连接自然就断开了

也就是说在目前的设计下,因为客户端和$request绑定,PSR风格下¥request因为总是新建的,如果不开启池, 所以客户端只请求一次就析构了。
如果参考guzzle的设计,客户端和request分开,客户端实例实际独立存在于request,这样只要同服务器,更换request会始终保持长连接开启,也和文档行为描述一致。期待2.0能朝着此方向重构。

@twose
Copy link
Member

twose commented Apr 12, 2019

@ihipop request上只是相当于保存了client的指针而已, 你需要create方法创建一个客户端, 然后再调用psr方法继续操作, 和request并没有长期的绑定关系

@ihipop
Copy link
Contributor Author

ihipop commented Apr 12, 2019

@twose

saber/src/SaberGM.php

Lines 17 to 25 in 5cd1134

private static function getDefaultClient(): Saber
{
return self::$defaultClient ?? self::$defaultClient = Saber::create();
}
public static function psr(array $options = []): Request
{
return self::getDefaultClient()->psr($options);
}

SaberGM的psr方法默认就会创建一个默认saber客户端,多次调用返回的是同一个,你保存在静态属性里面了。
我说的客户端不是saber客户端而是实际发送请求的客户端,也就是 $client_pool->createEx() 实际返回的swoole http 客户端

$this->client = $client_pool->createEx($options, !$this->use_pool);

他才负责具体的发送和长连接保持。

@twose
Copy link
Member

twose commented Apr 12, 2019

嗯 你是对的 这个应该考虑改动设计了

@ihipop
Copy link
Contributor Author

ihipop commented Apr 12, 2019

TL;DR 整理一下结论二:

  • Swoole Http Client(相当于Guzzle的具体负责执行请求的Handler)应该和Saber Client绑定
  • Request上和Swoole Http Client的绑定都应拆分到Saber Client上。
  • 连接池管理应该拆分到Saber Client
  • Request应该只负责描述请求。

@ihipop
Copy link
Contributor Author

ihipop commented Apr 18, 2019

实测,即使不使用PSR模式,只要不开启池,那么长连接都是不生效的,具体原因就是之前分析的那些,Request你每次都是新建的。请问,在不重构设计之前,接受PR文档修正增加配置描述还是PR代码补丁修复?
测试代码:

$testSaber = function () {
    $saber = \Swlib\Saber::create([
        'base_uri' => 'http://127.0.0.1:8081',
       // 'use_pool' => true,
        'headers'  => [
            'Accept-Language' => 'en,zh-CN;q=0.9,zh;q=0.8',
            'Content-Type'    => \Swlib\Http\ContentType::JSON,
            'DNT'             => '1',
            'User-Agent'      => null,
        ],
    ]);
    while (true) {
        if (!($saber->get('/anything'))) {
            echo 'FAILED' . PHP_EOL;
        };
    }
};
for ($i = 1; $i <= 2; $i++) {
    go(function () use ($testSaber) {
        $testSaber();
    });
}

@twose
Copy link
Member

twose commented Apr 19, 2019

可以暂时修改文档描述, 代码补丁会在下个版本

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants