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

Fix 435 #439

Merged
merged 4 commits into from
Sep 15, 2019
Merged

Fix 435 #439

merged 4 commits into from
Sep 15, 2019

Conversation

redboltz
Copy link
Owner

No description provided.

Added missing async_handler_t call on disconnect.
Removed error handling on do_sync_write().
If error happens the error will be handled by async_read.
If `handle_error(ec);` is called in `do_sync_write()`, error_handler
would be called twice.
It is consistent behavior as `do_async_write()`.
Removed redundant handler reset code from test_broker.
Added minimal support for Session Expiry Interval.
Timer is not set yet.
@codecov-io
Copy link

codecov-io commented Sep 14, 2019

Codecov Report

Merging #439 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
+ Coverage   84.78%   84.78%   +<.01%     
==========================================
  Files          40       40              
  Lines        6361     6364       +3     
==========================================
+ Hits         5393     5396       +3     
  Misses        968      968

@redboltz
Copy link
Owner Author

@jonesmz , this PR fixes std::shared_ptr<endpoint_t> resource management as the original design. When I introduced zero copy mechanism on v5.0.0, this bug has been en-bugged.

NOTE: Original design

On the server side such as broker, pass the lambda expression that captures shared_ptr<endpoint_t> to start_session() as async_handler_t as follows:

        ep.start_session(
            [sp] // keeping ep's lifetime as sp until session finished
            (boost::system::error_code const& /*ec*/) {
            }
        );

If ep is closed, then pointee of shared_ptr<endpoint_t> (ep) is deleted, so we don't need to clear handlers.

I will merge the PR. After I merged, please rebase #430 from the merged master.
Then, let's consider the lifetime issue again.

BOOST_TEST(sp == true);
// The previous connection is not set Session Expiry Interval.
// That means session state is cleared on close.
BOOST_TEST(sp == false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that if session expiry interval was not set, that means indefinite lifetime of the session data?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nervermind. I looked it up.

If the Session Expiry Interval is absent the value 0 is used. If it is set to 0, or is absent, the Session ends when the Network Connection is closed.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. no problem.

// set session_expiry_interval as infinity.
c->connect(
std::vector<MQTT_NS::v5::property_variant>{
MQTT_NS::v5::property::session_expiry_interval(0xFFFFFFFFUL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this infinity, or just maximum? It's a uint16_t, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's actually infinity.

3.1.2.11.2 Session Expiry Interval
17 (0x11) Byte, Identifier of the Session Expiry Interval.

Followed by the Four Byte Integer representing the Session Expiry Interval in seconds. It is a Protocol Error to include the Session Expiry Interval more than once.

If the Session Expiry Interval is absent the value 0 is used. If it is set to 0, or is absent, the Session ends when the Network Connection is closed.

If the Session Expiry Interval is 0xFFFFFFFF (UINT_MAX), the Session does not expire.

The Client and Server MUST store the Session State after the Network Connection is closed if the Session Expiry Interval is greater than 0 [MQTT-3.1.2-23].

test/resend.cpp Outdated
// TCP level disconnection detecting timing is unpredictable.
// Sometimes broker first, sometimes the client (this test) first.
// This test assume that the broker detects first, so I set timer.
// If client ditect the disconnection first, then reconnect with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditect = detect

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix all of them.

test/resend.cpp Outdated
// TCP level disconnection detecting timing is unpredictable.
// Sometimes broker first, sometimes the client (this test) first.
// This test assume that the broker detects first, so I set timer.
// If client ditect the disconnection first, then reconnect with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@@ -12,6 +12,27 @@ BOOST_AUTO_TEST_SUITE(test_resend_serialize_ptr_size)

using namespace MQTT_NS::literals;

template <typename Client>
inline void connect_no_clean(Client& c) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the same code in multiple files.

Can it be put in a header file?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will move it to header file.
I've checked existing header files but not found appropriate one. So I will create test_util.hpp.

@redboltz redboltz merged commit f28140c into master Sep 15, 2019
@redboltz redboltz deleted the fix_435 branch September 15, 2019 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants