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 issue#1874 #1964

Merged
merged 12 commits into from Nov 19, 2018

Conversation

@gilbertwong96
Copy link
Contributor

gilbertwong96 commented Nov 16, 2018

Prior to this change, if user use one client connect emqx with mqtt
v3.1.1, the client subscribe the topic and publish message to this
topic, it would receive this message itself published, this commit
provide a configure option to let user ignore the message itself published.

This change fix issue 1874

@gilbertwong96 gilbertwong96 requested review from spring2maz, tigercl, emqplus and terry-xiaoyu and removed request for tigercl Nov 16, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 16, 2018

Pull Request Test Coverage Report for Build 3817

  • 3 of 5 (60.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 60.989%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/emqx_plugins.erl 0 1 0.0%
src/emqx_protocol.erl 3 4 75.0%
Totals Coverage Status
Change from base Build 3809: 0.1%
Covered Lines: 2725
Relevant Lines: 4468

💛 - Coveralls
Prior to this change, if user use one client connect emqx with mqtt
v3.1.1, the client subscribe the topic and publish message to this
topic, it would receive this message itself published, this commit
provide a configure option to let user ignore the message itself published.

This change fix issue 1874.
@gilbertwong96 gilbertwong96 force-pushed the nolocal-option-for-old-spec branch from 18aeb48 to 0a47ab1 Nov 16, 2018
@gilbertwong96

This comment has been minimized.

Copy link
Contributor Author

gilbertwong96 commented Nov 16, 2018

I pushed a small fix commit, please squash merge it.

@gilbertwong96

This comment has been minimized.

Copy link
Contributor Author

gilbertwong96 commented Nov 18, 2018

This pull request still have bugs.

etc/emqx.conf Show resolved Hide resolved
TopicAlias =:= undefined orelse TopicAlias =< TopicAliasMaximum ->
case TopicAlias of
undefined ->
noreply(case emqx_zone:get_env(undefined, mqtt_ignore_loop_deliver, false) of

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 18, 2018

Contributor

although it is logically equivalent,
IMO, I think it's better to call emqx_config:get_env if it's not a per-zone config.

if
TopicAlias =:= undefined orelse TopicAlias =< TopicAliasMaximum ->
case TopicAlias of
undefined ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 18, 2018

Contributor

Does TopicAlias =:= undefined imply mqtt v3 ?
It would be confusing when we read this code again some time later or for people come across for the first time.
Ideally, the client should subscribe with nl set to true when proto_ver is lower than 5.0.
The least we can do here is to add a comment.

shutdown = Shutdown}) ->
terminate(SockError, _Req, #state{keepalive = Keepalive,
proto_state = ProtoState,
shutdown = Shutdown}) ->
?WSLOG(debug, "Terminated for ~p, sockerror: ~p",
[Shutdown, SockError], State),

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 18, 2018

Contributor

Perhaps not related to this PR. but the removal of State variable binding made me wonder why it compiled without error for this line.

This comment has been minimized.

Copy link
@gilbertwong96

gilbertwong96 Nov 18, 2018

Author Contributor

If retain the State, the compiler would warning that this Variable is unused.

@@ -382,10 +383,17 @@ process_packet(?PUBCOMP_PACKET(PacketId, ReasonCode), PState = #pstate{session =

process_packet(?SUBSCRIBE_PACKET(PacketId, Properties, RawTopicFilters),
PState = #pstate{session = SPid, mountpoint = Mountpoint, proto_ver = ProtoVer, is_bridge = IsBridge}) ->
IgnoreLoop = fun() -> case emqx_config:get_env(mqtt_ignore_loop_deliver, false) of

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 18, 2018

Contributor

it doesn't have to be a fun does it ?

priv/emqx.schema Show resolved Hide resolved
true -> [{RawTopic, SubOpts#{rap => 1}} || {RawTopic, SubOpts} <- RawTopicFilters];
false -> [{RawTopic, SubOpts#{rap => 0}} || {RawTopic, SubOpts} <- RawTopicFilters]
true -> [{RawTopic, SubOpts#{rap => 1, nl => IgnoreLoop()}} || {RawTopic, SubOpts} <- RawTopicFilters];
false -> [{RawTopic, SubOpts#{rap => 0, nl => IgnoreLoop()}} || {RawTopic, SubOpts} <- RawTopicFilters]

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 18, 2018

Contributor

so config default is false (when it's not set)
for mqtt 5.0, what happens if nl is set to true | 1 by subscriber?

This comment has been minimized.

Copy link
@gilbertwong96

gilbertwong96 Nov 19, 2018

Author Contributor

There's no effect on mqtt 5.0 message

@gilbertwong96

This comment has been minimized.

Copy link
Contributor Author

gilbertwong96 commented Nov 19, 2018

Too many small fix, please squash merge it.

gilbertwong96 and others added 5 commits Nov 19, 2018
Gilbert
@turtleDeng turtleDeng merged commit 1682149 into emqx30 Nov 19, 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.1%) to 60.989%
Details
@gilbertwong96 gilbertwong96 deleted the nolocal-option-for-old-spec branch Nov 19, 2018
tigercl added a commit that referenced this pull request Nov 24, 2018
* Fix issue#1874
Prior to this change, if user use one client connect emqx with mqtt
v3.1.1, the client subscribe the topic and publish message to this
topic, it would receive this message itself published, this commit
provide a configure option to let user ignore the message itself published.

This change fix issue 1874.

* Small Fix

* Fix bug

* Better design

* Fix compile warning and improve coverage

* Better design to solve the performance issue

* Fix typo

* Fix typo

* Delete spaces in end of lines.

* Do not use anonymous function

* Better performance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.