tarantool-php / client

PHP client for Tarantool.
MIT License
67 stars 22 forks source link

added feature to set socket timeout as float #71

Closed xBazilio closed 3 years ago

xBazilio commented 3 years ago

For highload projects 1 second is too long for a timeout. This simple change adds ability to set socket timeout as float. For example 0.1 is 100 miliseconds.

shmel1k commented 3 years ago

@rybakit , can you take a look, please?

rybakit commented 3 years ago

Thanks for the pull request, I think it's a good idea and you have a reasonable use case. Unfortunately, I can't merge it as is, because supporting float timeouts is a bit more work than tweaking the StreamConnection class. Besides socket_timeout, the connect_timeout option should also be changed to float, Client::fromDsn() and Client::fromOptions() should be adjusted accordingly, documentation should be updated, relevant tests should be added/improved, and so on. I will try to find some time to take care of these details.

xBazilio commented 3 years ago

Client::fromOptions() doesn't check option's type. So I didn't do anything whit it. I added necessary changes to Client::fromDsn(), to Readme and to tests. Did I miss anything else?

xBazilio commented 3 years ago

Also I see checks were failed due to something not related to my changes:

Detected deprecations in use:
- Configuration file `.php_cs.dist` is deprecated, rename to `.php-cs-fixer.dist.php`.
- PhpCsFixer\Config::create is deprecated since 2.17 and will be removed in 3.0.
Error: Process completed with exit code 8.

I didn't touch this file. I think it should be fixed separately.

xBazilio commented 3 years ago

I fixed php-cs-fixer deprecation errors in #72

rybakit commented 3 years ago

Also I see checks were failed due to something not related to my changes:

@xBazilio The workflow fails not because of the deprecation warning, but because of the code style, if you scroll a bit up, you'll see the expected formatting:

-        $socketTimeoutSeconds = (int)($this->options['socket_timeout']);
-        if (is_float($this->options['socket_timeout'])) {
-            $socketTimeoutMicroseconds = (int)($this->options['socket_timeout'] * 1000000);
+        $socketTimeoutSeconds = (int) ($this->options['socket_timeout']);
+        if (\is_float($this->options['socket_timeout'])) {
+            $socketTimeoutMicroseconds = (int) ($this->options['socket_timeout'] * 1000000);
rybakit commented 3 years ago

@xBazilio Please check #73. I decided to do a bit of refactoring of StreamConnection and enhanced tests for timeouts and option types. Please test it and let me know if it works for you, I plan to merge it this weekend.

Client::fromOptions() doesn't check option's type.

It did, but implicitly, for example, the method threw TypeError if you passed a non-string uri value or a non-integer max_retries. However, the timeouts were not checked, so I also fixed that.