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

ihipop commented 5 years ago

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

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

ihipop commented 5 years ago

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

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

twose commented 5 years ago

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

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

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

ihipop commented 5 years ago

我的意思也不是让你去做一个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 commented 5 years ago

二：客户端重用长连接

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

动图：

深度录屏_选择区域_20190411165555

twose commented 5 years ago

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

ihipop commented 5 years ago

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

twose commented 5 years ago

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

ihipop commented 5 years ago

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

ihipop commented 5 years ago

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

具体的问题代码如下：

https://github.com/swlib/saber/blob/5cd11340e39c335e3fc6f8fee38ebdbe16b57def/src/Request.php#L504-L516

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

twose commented 5 years ago

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

ihipop commented 5 years ago

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

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

twose commented 5 years ago

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

ihipop commented 5 years ago

@twose https://github.com/swlib/saber/blob/5cd11340e39c335e3fc6f8fee38ebdbe16b57def/src/SaberGM.php#L17-L25

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

https://github.com/swlib/saber/blob/5cd11340e39c335e3fc6f8fee38ebdbe16b57def/src/Request.php#L515

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

twose commented 5 years ago

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

ihipop commented 5 years ago

TL;DR 整理一下结论二：

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

ihipop commented 5 years ago

实测，即使不使用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 commented 5 years ago

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

swlib / saber

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