add _client->read() failure test

[hmueller01]

Resolves knolleary#988.
Thanks to @ajlennon.

[TD-er]

Looks like a good catch/fix.
I wonder how many crashes there may have been due to this...

However there is still something odd:

        if (currentMillis - previousMillis >= this->socketTimeout * 1000UL) {
            return false;
        }

Why consider the socketTimeout as seconds here?

It isn't a bug, just odd as you now need to multiply it with 1000 every time you want to check it.

[CONSULitAS]

Why consider the socketTimeout as seconds here?

This has been introduced with 2.4 and is defined als 15 (commented as 15 seconds). Only used once.

https://github.com/search?q=repo%3Ahmueller01%2Fpubsubclient3%20MQTT_SOCKET_TIMEOUT&type=code

[hmueller01]

Your are right. It's really dumb to always multiply socketTimeout by 1000. Could do this once when we set it.
Question is, if a uint16_t with a max. of 65535 resulting in 65s is enough, or if in that case socketTimeout should change to uint32_t (aka unsigned long) as well ...

[hmueller01]

Looks like a good catch/fix. I wonder how many crashes there may have been due to this...

I think almost none, as we only read from the client, if the client signals available(). But never now what might happen and this it definitely the better implementation.

[TD-er]

LGTM