writeToSocket(): You doing it wrong

[webloft]

The method \PhpMqtt\Client\MqttClient::writeToSocket() currently has an implementation of socket_set_blocking() to ensure correct writing:

This is wrong:

if ($this->settings->shouldUseBlockingSocket()) {
            socket_set_blocking($this->socket, false);
}

And this is wrong too:

if ($result === false || $result !== $length) {

Please referer to the php documentation to stream_set_blocking(), it says:

This affects calls like [fgets()](https://www.php.net/manual/en/function.fgets.php) and [fread()](https://www.php.net/manual/en/function.fread.php) that read from the stream. It says nothing about writing.

In addition it says for fwrite():

Writing to a network stream may end before the whole string is written. Return value of fwrite() may be checked and then shows a loop that calls fwrite() until all bytes are written.

Understanding the problem:

socket_set_blocking() sets the O_NONBLOCK flag. That means it will not block the local process from execution if it tries to read from or write to a receiver.

It has no impact how the data actually can be written over the line. This is made by a underlying I/O-controller. Sending more data than the receiver buffer can handle will result in broken pipe errors (the ones that are silenced via @-operator).

But even if fwrite() returns false, it could be just a temporary problem and must be retried. You may check stream_get_meta_data() for some stream state or even feof() if the connection was really closed.

Long story short: never rely on blocking sockets for write operations.

• Check the return value by fwrite()
• Repeat until all data was sent
• Don't stop after a "false"
• Only abort after a timeout or by other bad condition

[Namoshek]

The way this is currently implemented is due to multiple issues in the past, see #133, #135 and #141. Also check out #2 for a bit of history.

All of these bits are covered by tests (at least the parts that are testable), so I'm quite confident it is working as intended and the underlying functions have not been misused. (And my confidence in 19 years old comments in the PHP docs is kinda low to be honest...)

Also, have you seen this part in the settings?

https://github.com/php-mqtt/client/blob/1b1970142a3a165963537acd168b0fb65ceae1df/src/ConnectionSettings.php#L80-L96

[webloft]

To clarify: It's not misused, I'm writing about error cases.

Of course, setting blocking on the client will ensure that fwrite() will not just return if it could not send the data. But you still need to check if all data in your buffer was written.

On a looped library, there should be no blocking at sending, it should just buffer the data to send and see if it's there still data to be sent in the next cycle. I know, this stuff needs a fine logic.

According to #2 and related problems: you should simple use stream_select() to see if data is available or the socket has been closed.

[Namoshek]

I've looked into this in a bit more detail and I'm quite torn apart. On the one hand, I agree this would be good for the event driven part of the library. On the other hand, this makes it harder to properly implement the fire-and-forget publish part for QoS 0 which is also a significant feature. I've also noticed while implementing a draft that introducing an additional write buffer (which would be necessary in my opinion) breaks existing features like the auto-reconnect and also has major impact on the QoS 0 feature set.

Based on the issues and feedback I've received so far, the top use cases for this library are (in this order):

• RPC calls for IoT devices: very low-traffic and simple
• Publish with QoS 0 (i.e. fire and forget): very simple
• Subscribe with any QoS (doesn't make a big difference): rather complex from a PHP developers perspective and rarely used

Even though I would like the library to be the best implementation for all those use cases (and more), I fear it's impossible to provide a proper implementation and developer experience at the same time.

I currently also cannot provide the time required to make these changes. Though I'm very open to pull requests if you want to give it a try. Only requirement from my side is that existing tests must pass without changes and the public API of the client should not be touched at all if possible.

[webloft]

I see such changes only makes sense on a wide demand.

But, (I'm may wrong now):

publishMessage() claims: This is an internal method used for both, initial publishing of messages as well as re-publishing in case of timeouts.

This is true for QoS feedback but not true if writeToSocket() would just dismiss the message without a timeout option.

• non-blocking: may just fails if the broker is too busy
• blocking: should not fail, however if it does, the application is blocked infinitely (thinking of firewalls that just drop packets)

For this edge-case-scenario I think there should be the option to use non-blocking but with a custom timeout applied, so writeToSocket() just retries until the timeout is reached. Actually, it should also set the timeout option on the stream so it doesn't need to sleep.

I also noticed that the socket already has a timeout set by establishSocketConnection() that should be in effect while blocking too but I don't know if stream_set_timeout() works as expected in that case.