connect_options_builder doesn't propagate always properties

[Aposhian]

I just had an instance where I was setting properties on a mqtt::connect_options_builder, and then copying the builder, and then the properties didn't show up in the final connect request. Switching to using mqtt::connect_options directly resolved the issue.

So I was doing roughly:

mqtt::connect_options_builder builder = mqtt::connect_options_builder().version().properties(mqtt::properties{...});

mqtt::connect_options_builder builder2 = builder;

mqtt::connect_options options = builder2.more().chained().methods().finalize();

and then there were no properties on the final options object.

I'm guessing this has something to do with the properties object using a pointer to the properties that isn't updated on copy.

[fpagliughi]

Ha. I hadn’t even thought of anyone copying a builder!

But, obviously, it should either work or not be allowed. I’d be worried if it is because a Properties object is not copying correctly. I thought that behavior was unit tested.

But I’ll look into it.

[Aposhian]

Yeah I'm guessing it is caused by either copying the builder, or some implicit ordering requirements on the chained methods called on the builder. Or neither. :joy:.

[fpagliughi]

Yeah, the connect options wound up giving me headaches, particularly in the different settings for MQTT v3 and v5. The silliest is clean_start vs clean_session. The C library treats them as completely separate entities and will reject the options if the wrong flag is set for the requested version. Which is interesting... since they actually refer to the same bit in the CONNECT packet.

So I was trying to keep everything consistent, even when the user called set_mqtt_version() after setting some parameters. This was a fool's game, because that could have tossed away some of the options that the user already set. Confusion.

The latest thinking is to create the optoins/builder, specifying the desired version in the constructor, then not let the user change it. That makes life easier.

Anyway, that could have been part of the problem.

[fpagliughi]

Uuuuuuugggggghhhhh....

I think I see it now. I started writing some unit tests and I spotted the issue (I think). I believe it's essentially a dangling reference from the first line of your example:

mqtt::connect_options_builder builder = mqtt::connect_options_builder()
    .version()
    .properties(mqtt::properties{...});

The builder's .properties() method returns a reference to the object it is modifying, i.e. *this. But the object is the one returned from the constructor mqtt::connect_options_builder(), which isn't being stored anywhere, and is going out of scope?

So the expression returns a reference to an object that no longer exists, but since you declared the builder object to be constructed from that, the compiler hapily constructs an object from a reference to another object that it just destroyed?

And maybe that would work for POD objects, since there was no time for anything else to overwrite the memory, but since the dangling reference now has a dangling pointer to the properties heap memory, that makes it likely that the properties will be lost or garbage by the time the options are created from the builder.

Or maybe none of that is true.

Don't you love C++?

Anyway, what should work is to properly construct a builder, after which it can be modified then copied to another builder, then used to create options.

Something like this is being added to the unit tests:

// Construct the builder, and save to a variable
auto orgOptsBldr = connect_options_builder::v5();

// Modify it
orgOptsBldr
    .user_name(USER)
    .password(PASSWD)
    .properties(mqtt::properties{...})l

// Construct another one
connect_options_builder optsBldr;

// Copy assignment for the builder
optsBldr = orgOptsBldr;

// Use the second builder to create the options
connect_options opts = optsBldr.finalize();

[fpagliughi]

I'm pretty sure that was the issue. All the new unit tests are working. If not, please feel free to reopen this.