Implemented #756. #757

redboltz · 2020-12-10T03:28:16Z

No description provided.

codecov · 2020-12-10T03:44:53Z

Codecov Report

Merging #757 (c108bb8) into master (7ef56d1) will increase coverage by 0.04%.
The diff coverage is 88.63%.

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
+ Coverage   85.50%   85.55%   +0.04%     
==========================================
  Files          60       60              
  Lines        8521     8549      +28     
==========================================
+ Hits         7286     7314      +28     
  Misses       1235     1235

redboltz · 2020-12-10T04:28:27Z

@kleunen , please check this PR.

kleunen · 2020-12-10T06:48:34Z

@kleunen , please check this PR.

I will check in moment

kleunen

I have some feedback on the implementation. Please consider the feedback, i think this is important feature of the broker.

kleunen · 2020-12-10T07:39:11Z

include/mqtt/broker/offline_message.hpp

+ idx.begin(),
+ [&](auto& e) {
+ auto qos_value = e.pubopts_.get_qos();
+ e.send(ep);


Send is const method, so you don't need idx.modify ?

kleunen · 2020-12-10T07:52:22Z

include/mqtt/broker/session_state.hpp

+ return;
+ }
+ catch (packet_id_exhausted_error const& e) {
+ MQTT_LOG("mqtt_broker", warning)


Here again, the result of the publish is handled by an exception. This will probably be called less often, but I don't think it is exceptional that the packet_id can run out. Also a warning is generated, but the broker is able to handle this behaviour now. So nothing to warn about.

Ideally you want to generate the packet_id here by the broker, and call publish with the pre-generated packet id. This should be possible with optional acquire_unique_packet_id (see also comment below)

I think packet_id_exhausted is exceptional case. I understand it actually happens. Even if it handles well in the broker, finally run out memory and got bad_alloc. And in the broker exception happens just one time across threshold, then the message is go to offline_messages.
If packet_id_exhausted frequently happens, the network capacity design is broken.

But as you mentioned, optional version of packet_id acquisition is worth considering. Please wait a moment.

I agree that network design is broken. If client becomes unresponsive or disconnected, message may accumulate. Or if broker is very slow, also messages accumulate. Ideally the connection should timeout soon. Otherwise the broker may accumulate a lot of memory. But this is use case optimization i think. User should select qos and connection keepalive timeouts to prevent this. Or make option to set maximum of offline messages per session?

I agree that network design is broken. If client becomes unresponsive or disconnected, message may accumulate. Or if broker is very slow, also messages accumulate. Ideally the connection should timeout soon. Otherwise the broker may accumulate a lot of memory. But this is use case optimization i think. User should select qos and connection keepalive timeouts to prevent this. Or make option to set maximum of offline messages per session?

I'm not sure I answer your question but,

User can use Message Expiry Interval

User can set Receive Maximum https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901049, and broker also set it https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901083. It is bidirectional. (Not implemented yet)

User can use some kind of application level flow control

I think that there are many existing options.

So far, I don't have a plan to implement broker option to limit offline message (size count). But in the future, I will add it if it is really needed.

kleunen · 2020-12-10T07:52:36Z

include/mqtt/broker/session_state.hpp

- // used with some hypothetical "async_server" in the future.
- con_->publish(
+ publish(
+ ioc,


Why remove comment about async ? I was wondering if this could be an async operation or not ? Is there some reason why this is not async ?

I don't remove it. It moved to publish()

kleunen · 2020-12-10T07:52:42Z

include/mqtt/broker/offline_message.hpp

+ idx.pop_front();
+ }
+ }
+ catch (packet_id_exhausted_error const& e) {


This loop will run until exception is thrown (packet id exhausted). In my opinion, this is not "exceptional" behaviour. Also, handling an exception is more expensive / performance intensive operation than handling a return value.

Maybe have an optional acquire_unique_packet_id (called: acquire_unique_packet_id_no_except)

/** * @brief Acquire the new unique packet id. * If all packet ids are already in use, then throw packet_id_exhausted_error exception. * After acquiring the packet id, you can call acquired_* functions. * The ownership of packet id is moved to the library. * Or you can call release_packet_id to release it. * @return packet id */ std::optional<packet_id_t> acquire_unique_packet_id_no_except() { LockGuard<Mutex> lck (store_mtx_); if (packet_id_.size() == std::numeric_limits<packet_id_t>::max()) std::optional<packet_id_t>(); if (packet_id_master_ == std::numeric_limits<packet_id_t>::max()) { packet_id_master_ = 1U; } else { ++packet_id_master_; } ....

And have existing acquire_packet_unique_id call this method:

/** * @brief Acquire the new unique packet id. * If all packet ids are already in use, then throw packet_id_exhausted_error exception. * After acquiring the packet id, you can call acquired_* functions. * The ownership of packet id is moved to the library. * Or you can call release_packet_id to release it. * @return packet id */ packet_id_t acquire_unique_packet_id() { auto result = acquire_unique_packet_id_no_except(); if (!result) throw packet_id_exhausted_error(); return result.value();

Fix minior bugs.

redboltz · 2020-12-10T08:28:50Z

I forgot send_will fix. I just force pushed it.

kleunen · 2020-12-10T08:32:09Z

I forgot send_will fix. I just force pushed it.

Ah yes. I forget. There is also a cout in system test of broker. Can you remove?

redboltz · 2020-12-10T08:35:20Z

Can you remove?

Ok, I just done.

Removed cout messages. Used loop counter variable.

kleunen · 2020-12-10T09:54:05Z

include/mqtt/broker/offline_message.hpp

@@ -96,25 +96,85 @@ class offline_messages {
 catch (packet_id_exhausted_error const& e) {
 MQTT_LOG("mqtt_broker", warning)
 << MQTT_ADD_VALUE(address, &ep)
- << e.what();
+ << e.what()
+ << " : send the rest messages later.";
 }
 for (auto const& oflm : messages_) {


Remove this loop ? message_ is empty here, correct ?

I can't clearly define, but I mean this loop:

for (auto const& oflm : messages_) {

Ah, yes.
I forgot removing it at the earlier commit. But I think that it has been removed.

I think that all comments are reflected to the code.
If it works on yourr environment, I will merge the PR.

yes, i have no further comments. Please give me some time to test, i have some other duties i have to do before.

Ok, please let me know if you finish your test.

redboltz · 2020-12-10T10:04:33Z

I think that all comments are reflected to the code.
If it works on yourr environment, I will merge the PR.

kleunen · 2020-12-10T12:49:43Z

I am testing now, but I still periodicaly see this assertion:
./example/broker
broker: /home/pi/mqtt_cpp/include/mqtt/endpoint.hpp:9158: auto mqtt::endpoint<std::mutex, lock_guard, 2>::send_pubrel(mqtt::endpoint<std::mutex, lock_guard, 2>::packet_id_t, v5::pubrel_reason_code, v5::properties, boost::any)::(anonymous class)::operator()(type-parameter-0-0, const type-parameter-0-1 &) const [Mutex = std::mutex, LockGuard = lock_guard, PacketIdBytes = 2]: Assertion `ret.second' failed.
Aborted

I am publishing now with QoS 2. This is unrelated to this PR, so maybe I should make an issue ? I have no idea how to reproduce yet. It only happens rarely

redboltz · 2020-12-10T12:53:36Z

Yes, Please create the new issue.
This assertion is new to me in the series of debug.

Can I merge the PR ?

kleunen · 2020-12-10T12:58:05Z

Yes, merge PR and I create separate issue for other assertion

Implemented #756.

df8702a

redboltz mentioned this pull request Dec 10, 2020

Missing exception handling of 'packet_id_exhausted_error' exception #756

Closed

kleunen reviewed Dec 10, 2020

View reviewed changes

Added optional return version of packet_id acquisition.

704dcb7

Fix minior bugs.

redboltz force-pushed the add_packet_id_exhausted_support_for_broker branch from 3dcbae3 to 704dcb7 Compare December 10, 2020 08:28

redboltz force-pushed the add_packet_id_exhausted_support_for_broker branch from 04bc9de to 89a6401 Compare December 10, 2020 08:44

Fixed test.

c108bb8

Removed cout messages. Used loop counter variable.

redboltz force-pushed the add_packet_id_exhausted_support_for_broker branch from 89a6401 to c108bb8 Compare December 10, 2020 09:02

kleunen reviewed Dec 10, 2020

View reviewed changes

redboltz merged commit 88d0592 into master Dec 10, 2020

redboltz deleted the add_packet_id_exhausted_support_for_broker branch December 10, 2020 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented #756. #757

Implemented #756. #757

redboltz commented Dec 10, 2020

codecov bot commented Dec 10, 2020 •

edited

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

kleunen left a comment

kleunen Dec 10, 2020

redboltz Dec 10, 2020

kleunen Dec 10, 2020 •

edited

redboltz Dec 10, 2020

redboltz Dec 10, 2020 •

edited

redboltz Dec 10, 2020

kleunen Dec 10, 2020

redboltz Dec 10, 2020

kleunen Dec 10, 2020

redboltz Dec 10, 2020

kleunen Dec 10, 2020 •

edited

redboltz Dec 10, 2020

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

redboltz commented Dec 10, 2020

kleunen Dec 10, 2020

kleunen Dec 10, 2020

redboltz Dec 10, 2020

kleunen Dec 10, 2020

redboltz Dec 10, 2020

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

Implemented #756. #757

Implemented #756. #757

Conversation

redboltz commented Dec 10, 2020

codecov bot commented Dec 10, 2020 • edited

Codecov Report

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

kleunen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleunen Dec 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redboltz Dec 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleunen Dec 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

redboltz commented Dec 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

redboltz commented Dec 10, 2020

kleunen commented Dec 10, 2020

codecov bot commented Dec 10, 2020 •

edited

kleunen Dec 10, 2020 •

edited

redboltz Dec 10, 2020 •

edited

kleunen Dec 10, 2020 •

edited