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

Improve will message #1889

Merged
merged 18 commits into from Oct 27, 2018

Conversation

@tigercl
Copy link
Collaborator

tigercl commented Oct 9, 2018

The Server delays publishing the Client’s Will Message until the Will Delay Interval has passed or the Session ends, whichever happens first. If a new Network Connection to this Session is made before the Will Delay Interval has passed, the Server MUST NOT send the Will Message.

gilbertwong96 and others added 5 commits Oct 8, 2018
Prior to this change, Prior to this change, the validation for the mqtt5.0 publish packet
which both contains zero-length topic name and topic alias is wrong.

This change fix this bug.
Fix topic_name validation bug
@tigercl tigercl added this to the 3.0-rc.1 milestone Oct 9, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 9, 2018

Pull Request Test Coverage Report for Build 3681

  • 24 of 27 (88.89%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 63.476%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/emqx_sm.erl 2 3 66.67%
src/emqx_session.erl 12 14 85.71%
Files with Coverage Reduction New Missed Lines %
src/emqx_session.erl 1 71.85%
Totals Coverage Status
Change from base Build 3677: 0.3%
Covered Lines: 2732
Relevant Lines: 4304

💛 - Coveralls
src/emqx_protocol.erl Outdated Show resolved Hide resolved
when is_integer(Interval), Interval > 0 ->
SendAfter = integer_to_binary(Interval),
emqx_broker:publish(WillMsg#message{topic = <<"$delayed/", SendAfter/binary, "/", Topic/binary>>});
send_willmsg(WillMsg) ->
Session1 = list_to_binary(pid_to_list(Session)),

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 9, 2018

Contributor

Question: why would we need to have the pid in the topic ?
Pid tend to be ephemeral because it may crash/restart, make sense to have the client-id instead ?

This comment has been minimized.

Copy link
@tigercl

tigercl Oct 10, 2018

Author Collaborator

Yes, you are right, client id will be better. Session pid or client id is used to find and delete the will message if session resumed before the Will Delay Interval has passed.

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 10, 2018

Contributor

I can't seem to find the code (in this review diff) which tries to find the session back and delete something.

Copy link
Contributor

spring2maz left a comment

Looks good.
One last questions though, why the SendAfter integer should be a topic level ?

tigercl added 2 commits Oct 10, 2018
@tigercl tigercl removed the request for review from CrazyWisdom Oct 19, 2018
@@ -628,21 +638,24 @@ try_open_session(PState = #pstate{zone = Zone,
case emqx_sm:open_session(SessAttrs1) of
{ok, SPid} ->
{ok, SPid, false};
{ok, SPid, true} ->
emqx_delayed_publish:cancel_publish(ClientId),

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Oct 23, 2018

Contributor

Why only cancel the stored will message when Session Present == true? I think we need to "try to" delete the stored will message upon client connected, no matter if there is an existing session.

This comment has been minimized.

Copy link
@tigercl

tigercl Oct 24, 2018

Author Collaborator

If client connected but no existing session, you should send will message as usual.

src/emqx_protocol.erl Show resolved Hide resolved
tigercl added 5 commits Oct 25, 2018
…ean start and will message in connect packet
tigercl added 3 commits Oct 27, 2018
@turtleDeng turtleDeng merged commit 41b79e4 into emqx30 Oct 27, 2018
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 63.476%
Details
@turtleDeng turtleDeng deleted the improve_connect branch Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.