Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mqtt::connect_options_builder does not work when setting mqtt::properties #444

Closed
dmf254 opened this issue Jul 13, 2023 · 3 comments
Closed
Labels
bug Confirmed bug
Milestone

Comments

@dmf254
Copy link

dmf254 commented Jul 13, 2023

When using mqtt::connect_options_builder::set_properties, the mqtt::connect_options::opts_::connectProperties member is NULL after calling finalize. I believe there's some kind of bug present in the copy constructor/assignment for mqtt::connect_options.

// This does not work (conn_opts1.opts_.connectProperties == NULL)
mqtt::properties conn_props{mqtt::property{mqtt::property::SESSION_EXPIRY_INTERVAL, 600000}};
const auto conn_opts1 = mqtt::connect_options_builder().properties(conn_props).finalize();

// Works
mqtt::connect_options conn_opts2;
conn_opts2.set_properties(conn_props);

// Works
mqtt::connect_options conn_opts3(conn_opts2);

Environment Info:
Microsoft Visual Studio Professional 2022 (64-bit) - LTSC 17.4 (Version 17.4.7)
Windows SDK version 10.0.22000.0 to target Windows 10.0.19045.
MSVC_VERSION 1934 (v143 toolset)

@fpagliughi
Copy link
Contributor

Thanks for reporting this. I think I see what might be missing. First I'd like to try and write a unit test that catches the bug. Then I'll try the fix and and make sure I got it.

@fpagliughi fpagliughi added the bug Confirmed bug label Jul 14, 2023
@fpagliughi fpagliughi added this to the v1.3 milestone Jul 14, 2023
@ssams
Copy link
Contributor

ssams commented Jul 18, 2023

I stumbled across the same problem and tried to fix it already, only now also found the issue - definitely an issue in the constructors. PR in #445 should resolve it, let me know if further updates are needed.

@fpagliughi
Copy link
Contributor

I put in a simple unit test to check this, but I did so after I had already merged #445. The unit test is passing, so I will assume that it was #445 that fixed it, and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

3 participants