Warning error gets triggered in TLS mode

[zssarkany]

Hi all,

this package and the entire Amp ecosystem is really cool! :)

I found something worth mentioning. If we use the server with TLS enabled, it triggers warning error when client connects.

The relevant execution point in \Amp\Websocket\Server\Websocket->reapClient

if (\function_exists('socket_import_stream') && \defined('TCP_NODELAY')) {
    $sock = \socket_import_stream($socket->getResource());
    /** @noinspection PhpComposerExtensionStubsInspection */
    @\socket_set_option($sock, \SOL_TCP, \TCP_NODELAY, 1); // error suppression for sockets which don't support the option
}

The triggered warning message:

PHP Warning:  socket_import_stream(): cannot represent a stream of type tcp_socket/ssl as a Socket Descriptor in .../vendor/amphp/websocket-server/src/Websocket.php on line 207

The root cause is a really old, but still existing bug/thing. https://bugs.php.net/bug.php?id=70939

As I see after some doc-digging, stream* methods are the preferred over socket* methods, but it is impossible to get the underlying socket after stream_socket_accept to set tcp_nodelay in case of TLS-enabled server.

IMHO there are two improvements possibility:

• putting a single @ character before socket_import_stream to avoid triggering warning
• write to websocket documentation, that in case of tls, some delays may occur (because we are unable to disable Nagle's algorithm)

While this error is triggered, it makes integration of this cool package into Laravel projects quite tricky, because Laravel transforms warnings to exceptions by default to achieve better code quality. The only solution I found to make this package work with TLS and Laravel is to provide a custom error handler to set_error_handler(), which silently ignores this specific warning, than invokes the previous handler. This approach is obviously really hacky.

What are your thoughts?

[zssarkany]

After some more digging... :)

If we can believe in the results of stream_context_get_options() there are two ways to achieve tcp_nodelay.

1. Passing a BindContext with withTcpNoDelay() to \Amp\Socket\Server::listen()
2. using \stream_context_set_option($socket->getResource(), 'socket', 'tcp_nodelay', true); instead of stream_socket_import and socket_set_option

[kelunik]

@zssarkany Thanks for the report!

According to the comment above the mentioned code the stream API doesn't seem to work. I'm however not sure how we got that impression and whether this is actually correct. Apart from suppressing the error, we can also check the stream meta data and avoid the call if we have a TLS stream.

Changing the BindContext is obviously possible, but will set nodelay for any connection, while it might only be desired for connections upgraded to the WebSocket protocol.

[zssarkany]

@kelunik Thanks for the feedback!

I saw the comment you wrote about. I tested websocket-server v2.0.0-rc3 with PHP 7.2, and seemingly the stream api worked, therefore I was confused about the comment. In my env stream_context_get_options() result shows, that tcp_nodelay is true for socket wrapper. I don't know, if we can believe, that if the value is true, TCP_NODELAY is effective or not. Further tests were not performed by me.

Actually originally I was facing the issue using the stable v1.x ws server, therefore the same issue exists on that branch.

[kelunik]

@zssarkany I just confirmed it using strace, using stream_context_set_option doesn't work.

strace php examples/stackexchange-questions/server.php 2>&1 | grep setsockopt

[kelunik]

Adding support should be easy, but will obviously only help for newer versions of PHP: https://heap.space/xref/php-src/ext/openssl/xp_ssl.c?r=94e09bfe#2566.