Working with binary contents - how to convert std::vector<uint8_t> to a string_view?
DavidAntliff opened this issue · 14 comments
Hi Takatoshi,
Unless I'm mistaken, MQTT is considered suitable for the transfer of binary data, as well as text (strings). Therefore contents data may be an array of arbitrary byte values. For example, some bytes within the contents may have value zero. With that in mind, do you have any advice on how to use mqtt_cpp's API with std::vector<uint8_t> contents type, which seems more appropriate than std::string for binary data?
Looking at mqtt::allocate_buffer() overrides I see that they are all based around string-based contents (boost::string_view and iterable collections of strings). So in C++17 I've tried this (client_ is an instance of decltype(MQTT_NS::make_sync_client(std::declval<boost::asio::io_context &>(), "", 0));):
void MQTTClient::send_message(const std::string & topic, const std::vector<uint8_t> & contents) {
client_->socket()->post(
[&c = client_, &topic, &contents] {
mqtt::string_view sv {reinterpret_cast<const char *>(contents.data()), contents.size()};
c->publish(mqtt::allocate_buffer(topic),
mqtt::allocate_buffer(sv),
mqtt::qos::at_most_once);
}
);
}
Unfortunately this crashes with the throwing of a std::bad_alloc exception.
Although so does the following code, which surprises me because #831 (comment) seemed like the right way to handle the lifetimes of the topic and contents instances:
client_->socket()->post(
[&c = client_, &topic, &contents] {
c->publish(mqtt::allocate_buffer(topic),
mqtt::allocate_buffer("contents"),
mqtt::qos::at_most_once);
}
);
This makes me wonder if I've actually got two separate problems - correctly handling topic/contents lifetime, and handling conversion of std::vector<uint8_t>.
There are many types in C++. e.g.) std::byte. I don't think std::vector<uint8_t> is the best choice but it is not important.
If you want to use std::vector<uint8_t>, you can use
mqtt_cpp/include/mqtt/endpoint.hpp
Lines 1157 to 1168 in 3d045b8
You can create as::const_buffer by yourself using the following APIs:
https://www.boost.org/doc/libs/1_77_0/doc/html/boost_asio/reference/buffer.html
You need to manage its lifetime.
In terms of managing lifetime - what's the mechanism that your library uses to notify a buffer that it is no longer required? Are you using shared_ptr? Do you have any self-managing buffers that can copy the data and then delete themselves later? I thought that mqtt::allocate_buffer would do based on the comments around it, that but I can't get that to not crash.
I must admit I'm struggling with the API when using dynamic std::string instances - do you have a previous example somewhere of using a non-static std::string as the topic (or contents)? Almost all the examples I can find use strings created in main so naturally their lifetime lasts for the entire program.
mqtt::buffer and life_keeper parameter is related but different thing.
I got confused. mqtt:buffer should work well. Please show me a minimal and complete code that reproduces the crash.
The parameter life_keeper 's type is mqtt::any. As you mentioned, shared_ptr is a good candidate to pass as a life_keeper.
The life_keeper would be moved/copied (it is any's requirement). Caller needs to pass the type that continues lifetime if it is copied similar as shared_ptr.
void MQTTClient::send_message(const std::string & topic, const std::vector<uint8_t> & contents) {
client_->socket()->post(
[&c = client_, &topic, &contents] {
// topic, and contents would be dangling reference here if the lifetime of them is well managed outside of send_message()
// so size() would be any value if it is very huge value, then bad_alloc is thrown
mqtt::string_view sv {reinterpret_cast<const char *>(contents.data()), contents.size()};
c->publish(mqtt::allocate_buffer(topic),
mqtt::allocate_buffer(sv),
mqtt::qos::at_most_once);
}
);
}Possible solution:
void MQTTClient::send_message(const std::string & topic, const std::vector<uint8_t> & contents) {
mqtt::string_view sv {reinterpret_cast<const char *>(contents.data()), contents.size()};
client_->socket()->post(
[&c = client_, topic = mqtt::allocate_buffer(topic), sv = mqtt::allocate_buffer(sv)] () mutable {
c->publish(mqtt::force_move(topic),
mqtt::force_move(sv),
mqtt::qos::at_most_once);
}
);
}Thank you - your "Possible solution" does seem to work. Thank you for the explanation about dangling references.
Just out of curiosity, would you expect this related function to compile?
void MQTTClient::send_message(const std::string & topic, const std::string & contents) {
client_->socket()->post(
[&c = client_, topic, contents] {
c->publish(topic,
contents,
mqtt::qos::at_most_once);
}
);
}It doesn't compile for me, even though I would have thought that the lambda capture would make copies of topic and contents so that they are valid when the lambda runs, but the deleted ctor prevents this. The error is:
mqtt_cpp/include/mqtt/endpoint.hpp:962:32: error: use of deleted function ‘mqtt::buffer::buffer(std::string)’
962 | publish(packet_id, static_cast<buffer>(std::forward<T>(t)), std::forward<Params>(params)...);
So I applied the same principles that you used above and managed to get this to work:
void MQTTClient::send_message(const std::string & topic, const std::string & contents) {
client_->socket()->post(
[&c = client_, topic = mqtt::allocate_buffer(topic), contents = mqtt::allocate_buffer(contents)] () mutable {
c->publish(mqtt::force_move(topic),
mqtt::force_move(contents),
mqtt::qos::at_most_once);
}
);
}This seems quite convoluted and took me a long time to get to, so I want to check - is this how the API should be used with dynamic strings?
void MQTTClient::send_message(const std::string & topic, const std::string & contents) {
client_->socket()->post(
[&c = client_, topic, contents] {
c->publish(topic,
contents,
mqtt::qos::at_most_once);
}
);
}It seems that the code should be compiled (but doesn't work as expected).
mqtt::buffer is unrelated to the code.
Oh, I perhaps thought that mqtt::buffer was the one that publish tries to use to wrap the strings in self-managed buffers, but the deleted constructor makes this fail to compile. To be frank I'm not really sure what's going on as my understanding is limited.
So to be clear, are you saying that this should compile, but currently does not compile?
I mean currently compile.
Oh, this does compile for you?
My time is limited. I don't test it.
See https://stackoverflow.com/help/minimal-reproducible-example
Ok, my apologies, I appreciate the time you have spent already. I think you are saying that this should work as written? Then I will see what I can do to minimise my example.
I think you are saying that this should work as written?
I think that the code should be compiled. Otherwise it could be some hidden issue.
It is very welcome you reporting issues.
But it often happens the issue is caused by reporters code. It is not efficient to analyse.
So focusing on the suspicious part is very important.
In order to do that, you need to create a code that has complete include files hand int main() { ... }. In addition, its output. In this case it is compile error message. Also mqtt_cpp commit hash e.g. 70e45c2 is helpful.
I do that when I report a issue of the other OSS library. In my experience, during the process, I often found my code problem ;)
@redboltz you're absolutely right, attempting to construct a minimal example is often a good idea and often leads to solving the problem locally. And in this case that's true too.
To conclude this issue, and since you deserve to know, the problem was entirely on my end. For reasons I don't entirely understand, a single line of code in mqtt_cpp was inadvertently changed. A static_cast<buffer>() was placed around the first std::forward<T>(t) in one of publish() overrides. I suspect my IDE (CLion) tried to be clever and "fix" something. Anyway, this is what broke that publish function and cost me an entire day of wasted time.
Anyway, sorry to waste your time with this, I just wanted to let you know the outcome. And thank you again for your help and patience.