SESSION_EXPIRY_INTERVAL is int, not uint

[lizziemac]

According to the MQTT docs, it appears that the value should be an unsigned integer. When compiling with the following:

  auto opts_builder =
      mqtt::connect_options_builder()
          .v5()
          .properties({
            {mqtt::property::SESSION_EXPIRY_INTERVAL, UINT_MAX},
          })
          .clean_start(true)
          .automatic_reconnect(std::chrono::seconds(1),
                               std::chrono::seconds(MAX_RETRY_INTERVAL_SECONDS))
          .keep_alive_interval(std::chrono::seconds(30))
          .will(mqtt::message(LWT_TOPIC,
                              "{\"reason\": \"unexpected_disconnect\"}",
                              MQTT_QOS, false));

I get the error:

#23 5.172 error: narrowing conversion of '4294967295' from 'unsigned int' to 'int32_t' {aka 'int'} [-Wnarrowing]
#23 5.172    95 |       mqtt::connect_options_builder()
#23 5.172       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#23 5.172    96 |           .v5()
#23 5.172       |           ~~~~~       
#23 5.172    97 |           .properties({
#23 5.172       |           ~~~~~~~~~~~^~
#23 5.172    98 |             {mqtt::property::SESSION_EXPIRY_INTERVAL, UINT_MAX}
#23 5.172       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#23 5.172    99 |           })
#23 5.172       |           ~~   

This isn't the worst issue, but I think it is affecting how the broker handles it, as it's not truly set to the indefinite value.

[fpagliughi]

Yeah, you're right. It's complaining about the sign conversion.

I had assumed there might be a mix of signed and unsigned values in the properties. For each one, unsigned seemed to be implied, but when I just looked now, I see in the Data Representation section, it's explicit:

Four Byte Integer data values are 32-bit unsigned integers in big-endian order...

I'll change that in the code.

But I'm a bit worried about the spec assuming unsigned int is a 32-bit number. I mean that's pretty common now, but...

[lizziemac]

this is a bit out of my wheelhouse, but can we use uint32_t instead of unsigned int to protect 16 bit systems?

edit: I guess that UINT_MAX or UINT32_MAX wouldn't be compatible with the 16 bit systems, but hopefully in that scenario devs would just use const uint32_t INDEFINITE = 0xFFFFFFFF?

[fpagliughi]

Actually the problem is that I used the signed int32_t in several places in the properties code when I should have used uint32_t throughout for all 4-byte values. That also applies for 2-byte values. According to the spec (now that I bothered to actually read that part 🤦‍♂️) those should all be uint16_t. And the only variable-length integer property is also unsigned, though effectively up to 28 bits.

But, yes, int and unsigned int have no defined size in C/C++. Traditionally they were the machine word size for the target CPU: 16 bits on 16-bit CPUs, and 32 bits on 32-bit CPUs. The weird thing is that when the world moved to 64-bit CPU's the compilers kept int and unsized int at 32 bits! Weird. I guess the thinking was that billions of lines of code for PC's & Macs would break if they changed it.

But will it always stay the same? Who knows.

Better to be safe and use the sized types when you need a partucular size. In the case of C++, don't use the C macro for UINT_MAX. Better to use a sized constant like:

std::numeric_limits<uint32_t>::max()

It's a bit more verbose but guaranteed to be 0xFFFFFFFF.

Or make your own const as you suggest with a name more descriptive to the particular use case. INDEFINITE works there. Or maybe INFINITE.

[fpagliughi]

I just added this to the sync_consume_v5 example to make a const and test:

// Infinite time for session expiration
const uint32_t INFINITE = std::numeric_limits<uint32_t>::max();

...

auto connOpts = mqtt::connect_options_builder()
    .mqtt_version(MQTTVERSION_5)
    .automatic_reconnect(seconds(2), seconds(30))
    .clean_start(false)
    .properties({
        {mqtt::property::SESSION_EXPIRY_INTERVAL, INFINITE}
    })
    .finalize();

It's in the develop branch at the moment. I'll git it a closer look over, add some unit tests, and promote to master shortly.

[lizziemac]

Thanks so much!

[fpagliughi]

I believe this is fixed. If not, please re-open.

[lizziemac]

Finally was able to test, this works - thanks again!