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

Updated the parameter of set_accept_handler() from endpoint_t to std:… #443

Merged
merged 2 commits into from
Sep 16, 2019

Conversation

redboltz
Copy link
Owner

…:shared_ptr<endpoint_t>.

For set_*_handler(), capture std::weak_ptr<endpoint_t> at
test_broker and examples.

I think that weak_ptr is redundant. Reference is good enough because
shared_ptr lifetime is kept by start_session(life_keeper).

However, wp.lock() is easy to detect if the lifetime is over unexpectedly.

…:shared_ptr<endpoint_t>.

For `set_*_handler()`, capture `std::weak_ptr<endpoint_t>` at
test_broker and examples.

I think that `weak_ptr` is redundant. Reference is good enough because
`shared_ptr` lifetime is kept by `start_session(life_keeper)`.

However, `wp.lock()` is easy to detect if the lifetime is over unexpectedly.
@redboltz redboltz force-pushed the accept_cb_shared_ptr branch from 8805cdd to 4f158f2 Compare September 15, 2019 13:42
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@10830cd). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #443   +/-   ##
=========================================
  Coverage          ?   84.94%           
=========================================
  Files             ?       40           
  Lines             ?     6342           
  Branches          ?        0           
=========================================
  Hits              ?     5387           
  Misses            ?      955           
  Partials          ?        0

@jonesmz
Copy link
Contributor

jonesmz commented Sep 15, 2019

Are you certain that

close_proc(connections, subs, wp.lock());

is sufficient? wp.lock() may return nullptr

@redboltz
Copy link
Owner Author

redboltz commented Sep 15, 2019

Yes I'm certain that wp.lock() never return nullptr at close handler.

I inserted the following BOOST_ASSERT:

BOOST_ASSERT(wp.lock());
close_proc(connections, subs, wp.lock());

then no assertion failure occurred.

Let me explain why I'm certain about that.

First, the lifetime of the session (shared_ptr) is copy captured by start_session(). It is mandatory convention.

            ep.start_session(std::make_tuple(std::move(spep), std::move(g)));

wp.lock() is called from the close handler.

close_proc(connections, subs, wp.lock());

is called from h_close_().

h_close_() is called from handle_close().
https://github.com/redboltz/mqtt_cpp/blob/accept_cb_shared_ptr/include/mqtt/endpoint.hpp#L7644-L7646

handle_close() is called from handle_close_or_error()
https://github.com/redboltz/mqtt_cpp/blob/accept_cb_shared_ptr/include/mqtt/endpoint.hpp#L7598-L7634
at the line
https://github.com/redboltz/mqtt_cpp/blob/accept_cb_shared_ptr/include/mqtt/endpoint.hpp#L7626.

handle_close_or_error() is called from the lambda expression at handle_remaining_length() and check_error_and_transferred_length().

At handle_remaining_length() lambda expression, session_life_keeper is held by its capture list. So the lifetime of the session is enable.
https://github.com/redboltz/mqtt_cpp/blob/accept_cb_shared_ptr/include/mqtt/endpoint.hpp#L8055

check_error_and_transferred_length() is called from various places and session_life_keeper is enable at all places similar to handle_remaining_length().

So wp->lock() never return nullptr.

Refactored test_broker parameter passing.
@redboltz redboltz merged commit a17c6b2 into master Sep 16, 2019
@redboltz redboltz deleted the accept_cb_shared_ptr branch September 16, 2019 13:40
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