Use of boost:asio::buffer in as_buffer_pubsub.cpp

[aholzinger]

Hi,

I think the use of boost:asio::buffer in as_buffer_pubsub.cpp isn't correct like this: as::buffer(std::string("topic1")) as the buffer is just a representation of an externally supplied buffer and the temporary std::string is going out of scope while the boost:asio::buffer is still in use.

I would change the code like this: const std::string t1("topic1"); as::buffer(t1)

Cheers Axel

[aholzinger]

Here's a proposed patch: as_buffer_pubsub.cpp.patch.zip

[redboltz]

as_buffer_pubsub.cpp is test for synchronous APIs. That means the function no longer access to the buffer after the function is returned.

For example, https://github.com/redboltz/mqtt_cpp/blob/master/test/as_buffer_pubsub.cpp#L44

                    pid_sub = c->subscribe(as::buffer(std::string("topic1")), mqtt::qos::at_most_once);

The lifetime of std::string("topic1") can be over after subscribe() is returned.

I wrote a PoC code to check the lifetime of the string.

https://wandbox.org/permlink/dqosfBxmeqglBkwr

#include <boost/asio.hpp>
#include <boost/asio/buffer.hpp>
#include <iostream>
#include <string>

struct life_traced_string : std::string {
    template <typename T>
    explicit life_traced_string(T&& t):std::string(std::forward<T>(t)) {
        std::cout << __PRETTY_FUNCTION__ << std::endl;
    }
    life_traced_string(life_traced_string const&) = delete;
    life_traced_string(life_traced_string&&) = delete;
    life_traced_string& operator=(life_traced_string const&) = delete;
    life_traced_string& operator=(life_traced_string&&) = delete;
    ~life_traced_string() {
        std::cout << __PRETTY_FUNCTION__ << std::endl;
    }
};

inline void foo(boost::asio::const_buffer b) {
    std::cout << __PRETTY_FUNCTION__ << std::endl;
    (void)b;
    std::cout << "accessing buffer" << std::endl;
    std::cout << "finish:" << __PRETTY_FUNCTION__ << std::endl;
}    

int main() {
    std::cout << "before" << std::endl;
    foo(boost::asio::buffer(life_traced_string("ABC")));
    std::cout << "after" << std::endl;
}

Output:

before
life_traced_string::life_traced_string(T &&) [T = char const (&)[4]]
void foo(boost::asio::const_buffer)
accessing buffer
finish:void foo(boost::asio::const_buffer)
life_traced_string::~life_traced_string()
after

That indicates the lifetime of string is kept during the function foo(). So I think that the current code is not harmful.

What do you think?

By the way, I think that as::buffer(std::string("topic1")) can be replaced as as::buffer("topic1"). And it is simpler. In this case, "topic1" has static storage duration. I think that it is better. But I still think that the original code doesn't violate std::string's lifetime.

[jonesmz]

The original code (being the synchronous version) doesn't violate std::string's lifetime.

Using as::buffer("topic1") is better, because it doesn't involve an allocation and copy of the string data (Well, unless short string optimization is happening, but that's not guaranteed) and instead only references constant data from the binaries "read-only" section.

[aholzinger]

The problem is here:

#include <tuple>
#include <vector>
#include <boost/asio/buffer.hpp>

constexpr std::uint8_t const at_most_once = 0;

void func(std::vector<std::tuple<boost::asio::const_buffer, std::uint8_t>> params) {
    return;
}

int main() {
    func(std::vector<std::tuple<boost::asio::const_buffer, std::uint8_t>> {
        std::make_tuple(boost::asio::buffer(std::string{ "topic" }), at_most_once)
    });
}

This is happenning at runtime:

1. basic_string::basic_string
2. boost::asio::buffer
3. make_tuple
4. tuple::tuple
5. basic_string::~basic_string
6. vector::vector
7. func

After a lot of googling I found this: https://developercommunity.visualstudio.com/content/problem/334025/problem-with-lifetime-of-some-specific-temporary-c.html It seems to be a VS compiler bug. If this is the case I want to excuse me for making noise which would have been better to be made in Redmond, sorry for that.

Now regarding C-style-string and/or std::string:

const std::string str{ "topic1" };
auto buf1 = boost::asio::buffer(str);

Debugger: boost::asio::constbuffer = {data=0x008ffb18 size_=6 } memory 0x008ffb18: 74 6f 70 69 63 31 = topic1 (the next byte is 00 = \0)

auto buf2 = boost::asio::buffer("topic2");

Debugger: boost::asio::constbuffer = {data=0x00f95124 size_=7 } memory 0x00f9512418: 74 6f 70 69 63 32 00 = topic2\0

The two objects differ by the \0 byte.

What would that mean for the mqtt messages?

Cheers Axel

[redboltz]

@aholzinger , thank you for the great investigation. MSVC is headache ;)

I considered some approaches. See the following code:

Running demo: https://wandbox.org/permlink/zfsfSNof2P6b7O0K

#include <boost/asio.hpp>
#include <string>
#include <iostream>
#include <cstring>

void print_size(boost::asio::const_buffer b) {
    std::cout << b.size() << std::endl;
}

int main() {
    print_size(boost::asio::buffer(std::string("ABC")));        // bump MSVC bug
    print_size(boost::asio::buffer("ABC"));                     // '\0' is added. Unnatural for communication
    print_size(boost::asio::buffer("ABC", sizeof("ABC") - 1));  // "ABC" appears twice
    print_size(boost::asio::buffer("ABC", std::strlen("ABC"))); // "ABC" appears twice and need parsing
    print_size(boost::asio::buffer(std::string_view("ABC")));   // I think the best one.
}

I think that the last one is the best. Could you test the string_view approach on your environment? mqtt_cpp requires C++14, string_view is C++17 feature but boost has similar one. mqtt_cpp privides mqtt::string_view and you can use it. See https://github.com/redboltz/mqtt_cpp/wiki/Config#mqtt_std_string_view

[aholzinger]

I tested all possibilities on my platform.

Unfortunately boost has two times string_view:

1. boost::utility:string_view from boost/utility/string_view.hpp
2. boost::asio::string_view from boost/asio/detail/string_view.hpp

The former doesn't work with boost::asio::buffer. The latter is available since Boost 1.66.0, but unfortunately wants to use std::stringview or experimental::stringview. On my system (VS2017) with Boost 1.66.0 boost::asio::string_view is not working (error C2039: 'string_view': is not a member of 'boost::asio').

Unfortunately mqtt_cpp in include/mqtt/string_view.hpp is using boost::utility:string_view and thus also mqtt::string_view is not working, because boost::asio::buffer doesn't accept this type.

On VS2017 which I'm using, std::string_view with C++17 is working fine.

Is it worth it to force the whole project to C++17 for just four lines of test code? Untill the MS compiler bug isn't fixed I would use the C-style-string-strlen approach in the 4 locations (clearly with some comment why this approach and why the second parameter with strlen) and let the rest with (less efficient) std::string.

[redboltz]

Currently mqtt_cpp support Boost 1.57.0 or later. https://github.com/redboltz/mqtt_cpp#overview

Is it worth it to force the whole project to C++17 for just four lines of test code?

Yes. Could you send the pull request that uses like as follows?

#if defined(BUGGY_MSVC_VERSION)
                    // workaround for https://developercommunity.visualstudio.com/content/problem/334025/problem-with-lifetime-of-some-specific-temporary-c.html
                    pid_sub = c->subscribe(as::buffer("topic1", sizeof("topic1") - 1), mqtt::qos::at_most_once);
#else  // defined(BUGGY_MSVC_VERSION)
                    pid_sub = c->subscribe(as::buffer(std::string("topic1")), mqtt::qos::at_most_once);
#endif // defined(BUGGY_MSVC_VERSION)

Replace BUGGY_MSVC_VERSION with something appropriate one. See https://github.com/redboltz/mqtt_cpp/wiki/Coding-Rules

[aholzinger]

Will do so next week, but it will be difficult to find a valid define (_MSC_VER) as to date all C++14 compatible MS compilers have this issue. I checked also with latest VS2019 (16.1.3): same problem.

MS in the bug report said it would be fixed in 16.1 which is already out, so I guess it would be 16.2 then. 16.1.3 has _MSC_VER = 1921. Current V2017 is 15.9.13 and has _MSC_VER = 1916.

So a solution could be to check for _MSC_VER being lower than 1922. But it's not sure.

[redboltz]

I understand that checking all MSVC version are hard. This is an workaround for test codes. I think that only checking defined(_MSC_VER) is ok.

[aholzinger]

I filed a pull request (my first one, so please excuse if I made some mistakes).

I didn't compare against different MS compiler versions as all C++14 capable MS compilers suffer from this bug, so I compared against 1921 (<= 1921) which is the most current one.

Let's hope that 1922 will already have the bug fixed.

[redboltz]

Thank you.