amphp / http-client

An advanced async HTTP client library for PHP, enabling efficient, non-blocking, and concurrent requests and responses.
https://amphp.org/http-client
MIT License
702 stars 67 forks source link

Loop exceptionally stopped + Trying to access offset on value of type null #272

Closed duronrulez closed 4 years ago

duronrulez commented 4 years ago

using amphp/http-client: 4.4.0 and php 7.4.0

Reproducable script:

$builder = (new HttpClientBuilder);

        $client = $builder->build();

        $promise = call(
                function () use ($client) {
                    $url = 'https://samsung.com';

                    $request = new Request($url);
                    $response = yield $client->request($request);
                }
            );
        wait($promise);

Result:

Loop exceptionally stopped without resolving the promise {"exception":"[object] (Error(code: 0): Loop exceptionally stopped without resolving the promise at vendor/amphp/amp/lib/functions.php:232)
.....
[previous exception] [object] (ErrorException(code: 0): Trying to access array offset on value of type null at vendor/amphp/socket/src/Internal/functions.php:131)
[stacktrace]

I've also posted a similar issue in the amphp/socket repo, since the exception seems to be originating there: https://github.com/amphp/socket/issues/76

kelunik commented 4 years ago

It outputs a proper error message for me: PHP Fatal error: Uncaught Amp\Socket\TlsException: TLS negotiation failed: stream_socket_enable_crypto(): SSL operation failed with code 1. OpenSSL Error messages: error:141A318A:SSL routines:tls_process_ske_dhe:dh key too small. I guess it can be resolved by reducing the security level on the TLS context.

I've applied a patch to amphp/socket that should now report Unknown error to you if there's no available message.

duronrulez commented 4 years ago

Hm, thats weird, do you have any idea why i might be getting different results? Could it be related to openssl versions?

kelunik commented 4 years ago

Something like that might be the case, yes.

duronrulez commented 4 years ago

reducing the security level to 1 works, leaving this here for someone else https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_security_level.html

Now trying to understand why i dont get the same errors reported.


EDIT: OK, after removing the silence on stream_socket_enable_crypto, it does show the same error (as fatal, not exception), but i dont understand why its not being intercepted in error_get_last.

EDIT2: error_get_last may not contain the error if set_error_handler is used somewhere in the codebase. Hopefully this helps some poor soul.

kelunik commented 4 years ago

@duronrulez This means we should probably switch to using an error handler instead of error_get_last().

duronrulez commented 4 years ago

can confirm that if i put restore_error_handler() before the stream_socket_enable_crypto() call the error is being received in error_get_last(). In my case the handler is the default laravel exception handler: https://github.com/laravel/framework/blob/0b12ef19623c40e22eff91a4b48cb13b3b415b25/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php

Obviously this doesn't solve anything, but at least i know whats breaking it.

kelunik commented 4 years ago

Please try with https://github.com/amphp/socket/commit/75321ab97d189d00091b13db4c578a0bae024c2c.

duronrulez commented 4 years ago

Looks like its working 👍 All 3 contained exceptions are now being reported properly and catchable.

Not sure what is the process exactly, but when do you think the change will be released in a version?

kelunik commented 4 years ago

Tagged as v1.1.3. ;-)