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

Request & Response (broker and client) #1819

Merged
merged 41 commits into from Oct 18, 2018

Conversation

@gilbertwong96
Copy link
Contributor

gilbertwong96 commented Sep 14, 2018

To fully implement the response and request feature in MQTT 5, main job is to implement these two features in emqx_client. The implementation of the response & request features on broker has been done. Please review my code. Thanks.

Prior to this change, there is no validate and specified process for
Request-Response-Information and Response-Information
Prior to this change, there is no validation for response-topic which
may cause unexpected error when broker is forwarding request response
message.
Prior to this change, there are no test case for the new properties
validation added after beta1 release. It is not a best practice.
Prior to this change, there are few compile warning when emqx test
case is running

This change fix these problems.
@gilbertwong96 gilbertwong96 requested review from tigercl, emqplus and huangdan and removed request for tigercl and emqplus Sep 14, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2018

Pull Request Test Coverage Report for Build 3600

  • 115 of 139 (82.73%) changed or added relevant lines in 4 files are covered.
  • 19 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+1.6%) to 58.793%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/emqx_packet.erl 5 7 71.43%
src/emqx_protocol.erl 20 24 83.33%
src/emqx_client.erl 84 102 82.35%
Files with Coverage Reduction New Missed Lines %
src/emqx_cm.erl 1 69.39%
src/emqx_broker_helper.erl 1 47.06%
src/emqx_sys_mon.erl 1 18.33%
src/emqx_sm.erl 1 67.06%
src/emqx_router_helper.erl 2 26.09%
src/emqx_packet.erl 4 69.88%
src/emqx_protocol.erl 9 69.29%
Totals Coverage Status
Change from base Build 3595: 1.6%
Covered Lines: 2524
Relevant Lines: 4293

💛 - Coveralls
?assertEqual(true, emqx_packet:validate(?CONNECT_PACKET(#mqtt_packet_connect{}))).
?assertEqual(true, emqx_packet:validate(?CONNECT_PACKET(#mqtt_packet_connect{}))),
?assertEqual(true, emqx_packet:validate(?PUBLISH_PACKET(1, <<"topic">>, 1, #{'Response-Topic' => <<"responsetopic">>, 'Topic-Alias' => 1}, <<"payload">>))),
{'EXIT', {subscription_identifier_invalid, _Stacktrace}} =

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 14, 2018

Contributor

?assertException or ?assertExit

src/emqx_packet.erl Show resolved Hide resolved
@@ -208,9 +208,21 @@ received(Packet = ?PACKET(Type), PState) ->
true ->
{Packet1, PState1} = preprocess_properties(Packet, PState),
process_packet(Packet1, inc_stats(recv, Type, PState1));
{'EXIT', {topic_filters_invalid, _Stacktrace}} ->
{'EXIT', {protocol_error, _Stacktrace}, PState} ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 14, 2018

Contributor

As I commented in another PR.
Direct catch of an exception should consider deprecated and never use again.
The reason being:
unlike try ... catch _ : _
catch Expr will always try to retrieve the stacktrace which is very expensive.
especially in our case here --- exception driven implementation,
stacktrace is always discarded, hence no need to retrieve at all.

test/emqx_packet_SUITE.erl Outdated Show resolved Hide resolved
@gilbertwong96

This comment has been minimized.

Copy link
Contributor Author

gilbertwong96 commented Sep 14, 2018

Thanks for your instruction and advice @spring2maz

Prior to this change, emqx directly catch validate exception which
would try to retrieve the whole stacktrace. It would cost a lot of
resources and slow down the whole broker.

This change fix ths performance issue.
@spring2maz

This comment has been minimized.

Copy link
Contributor

spring2maz commented Sep 17, 2018

coverage degrade alert!

{'EXIT', {protocol_error, _Stacktrace}, PState} ->
process_packet(Packet1, inc_stats(recv, Type, PState1))
catch
error:protocol_error:_Stacktrace ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 17, 2018

Contributor

if it is to discard stacktrace like this (_Stacktrace).
there is no need to bind it.

This comment has been minimized.

Copy link
@gilbertwong96

gilbertwong96 Sep 17, 2018

Author Contributor

Yes

emqx_packet:validate(
?SUBSCRIBE_PACKET(15, #{'Subscription-Identifier' => -1},
[{<<"topic">>, #{qos => ?QOS0}}]))),
?assertException(error, topic_filters_invalid,

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 17, 2018

Contributor

?assertError is even better.

Prior to this change, if there are connect packets of mqtt 5.0
version and the packet is inconsistent with the connect packet
as mqtt 5.0 specification, it would not deliver reason code back to client

This change fix this bug
Prior to this change, there are serveral bugs in response&request
realized in broker and the protocol test cases are really bad.

This change fix the bugs and delete all the test cases in
emqx_protocol_SUITE.erl, also add connect test cases for    mqtt
4 and mqtt5.

-include_lib("eunit/include/eunit.hrl").

-import(emqx_serializer, [serialize/1]).
-include_lib1("common_test/include/ct.hrl").

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 18, 2018

Contributor

include_lib1?

clean_start = false,
password = <<"public">>})).

receive_messages(Count) ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 18, 2018

Contributor

Where is this function used?

This comment has been minimized.

Copy link
@gilbertwong96

gilbertwong96 Sep 18, 2018

Author Contributor

I push code to let my collegue see the code, there are still many problems in this code. I know that.

receive_messages(Count) ->
receive_messages(Count, []).

receive_messages(0, Msgs) ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 18, 2018

Contributor

Msgs is always returned as it is passed in. Why pass it in at all?

This comment has been minimized.

Copy link
@gilbertwong96

gilbertwong96 Sep 18, 2018

Author Contributor

It is draft.😂

Prior to this change, there is no such feature, I add this feature but still need to be tested.
is_assigned = IsAssigned}) ->
case maps:find('Request-Response-Information', CONNPROPS) of

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 21, 2018

Contributor

nit: avoid binding variables in case clauses and use them outside.
bind the case expression result to variable instead.
i.e

ResponseInfomation = case maps:find(....) of
    {ok, 1} -> ClientId;
    _ -> <<>>
end.

This comment has been minimized.

Copy link
@gilbertwong96

gilbertwong96 Sep 21, 2018

Author Contributor

Yes, you are right. :)

@@ -27,7 +27,7 @@

-record(ssl_socket, {tcp, ssl}).

-type(socket() :: inet:socket() | #ssl_socket{}).
%% -type(socket() :: inet:socket() | #ssl_socket{}).

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 21, 2018

Contributor

nit: delete.

props = NewProperties,
payload = iolist_to_binary(Payload)}),
Ref = erlang:start_timer(TimeOut, self(), response),
receive_response(Client, ClientId, Ref).

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 10, 2018

Contributor

wrap this call around a try after.

try
    receive_response(Client, ClientId, Ref)
after
    erlang:cancel_timer(Ref),
    receive {timeout, Ref, _} -> ok after 0 -> ok end, % flush
    erlang:demonitor(MRef, [flush])
end.

-type(response_topic() :: ?SHARED(ShareGroup :: binary(), binary())).

-export_type([client/0, topic/0, qos/0, properties/0, payload/0,

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 11, 2018

Contributor

This export_type exists before this change.
but it seems wrong to export qos/0, topic/0, packet_id/0 (maybe more) from this module.
these types seem to be common enough to be defined in and exported from emqx.erl.
topic/0 is defined as emqx_topic:topic() which is fair, but worth to expose it from emqx.erl too.
i.e. in emqx.erl: -type topic() :: emqx_topic:topic().

Also, some of the types (e.g. response_payload) is used internally only, so there is no need to export.


-type(response_topic() :: ?SHARED(ShareGroup :: binary(), binary())).

-export_type([client/0, topic/0, qos/0, properties/0, payload/0,

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 11, 2018

Contributor

it's a good practice to define -export_type right after the function -export section.

Prior to this change, there are no macro for <<>> which would make
code chaos and no monitor for client process.

This change adds macros for <<>>, move export_type under export, add
response_topic() type and delete duplicated exported types.
Prior to this change, there is a bug when exception happens and the
api is mixed fashion

This change fixed this bug and refactor api.
@gilbertwong96 gilbertwong96 force-pushed the Response&Request branch from 55f2cf5 to e4dcb74 Oct 12, 2018
case new_response_topic(Client, Topic) of
{ok, {NewResponseTopic, Properties}} ->
{ok, _Props, _QoS} = subscribe(Client, [{NewResponseTopic, [{rh, 2}, {rap, false},
{nl, true}, {qos, RequestQoS}]}]),

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 12, 2018

Contributor

According to spec:

It is a Protocol Error to set the No Local bit to 1 on a Shared Subscription

Does that mean we can not set {nl, true} here if shared?

@spring2maz

This comment has been minimized.

Copy link
Contributor

spring2maz commented Oct 12, 2018

lack of a shared subscription test case ?

spring2maz added 2 commits Oct 12, 2018
Prior to this change emqx_client:request APIs need
to make several gen_statem calls in order to get the request/response
topic prefix stored in client state data.

This commit tries to:
1. make less gen_statem call to get topic prefix
2. move the gen_statem call to a separate call
   so make the topic construction functions side-effect free
Also fixed a typo '$shared' -> '$share'
Enabled all group tests in emqx_client_SUITE
Also removed zero_length_clientid_test
@spring2maz spring2maz force-pushed the Response&Request branch from 29d552e to 338c4f6 Oct 13, 2018
is_assigned = IsAssigned}) ->
ResponseInformation = case maps:find('Request-Response-Information', CONNPROPS) of
{ok, 1} ->
iolist_to_binary(emqx_config:get_env(response_topic_prefix, "ResponseTopic"));

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 18, 2018

Contributor

this default value is not the same as in config. we should perhaps not allow a default value here ?

@@ -481,7 +505,14 @@ deliver({connack, ReasonCode}, PState) ->
deliver({connack, ?RC_SUCCESS, SP}, PState = #pstate{zone = Zone,
proto_ver = ?MQTT_PROTO_V5,
client_id = ClientId,
conn_props = CONNPROPS,

This comment has been minimized.

Copy link
@tigercl

tigercl Oct 18, 2018

Collaborator

CONNPROPS -> ConnProps

Feng Lee
##
## Value: String
## Default: 1MB
mqtt.response_topic_prefix = ResponseTopic

This comment has been minimized.

Copy link
@emqplus

emqplus Oct 18, 2018

Contributor
mqtt.response_topic_prefix = $response/

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 18, 2018

Contributor

this prefix is essentially a topic prefix to be used by clients. according to mqtt spec:

The Server SHOULD prevent Clients from using such Topic Names to exchange messages with other Clients. Server implementations MAY use Topic Names that start with a leading $ character for other purposes.

@@ -597,6 +597,12 @@ end}.
%% MQTT Protocol
%%--------------------------------------------------------------------

%% @doc Response Topic Prefix
{mapping, "mqtt.response_topic_prefix", "emqx.response_topic_prefix",[
{default, "ResponseTopic"},

This comment has been minimized.

Copy link
@emqplus

emqplus Oct 18, 2018

Contributor
{default, "$response/"}
@emqplus emqplus requested review from emqplus and removed request for huangdan Oct 18, 2018
Makefile Outdated
emqx_access emqx_broker emqx_cm emqx_frame emqx_guid emqx_inflight emqx_json \
emqx_keepalive emqx_lib emqx_metrics emqx_misc emqx_mod emqx_mqtt_caps \
emqx_mqtt_compat emqx_mqtt_props emqx_mqueue emqx_net emqx_pqueue emqx_router \
emqx_sm emqx_tables emqx_time emqx_topic emqx_trie emqx_vm emqx_mountpoint \

This comment has been minimized.

Copy link
@spring2maz

spring2maz Oct 18, 2018

Contributor

indent

@spring2maz spring2maz merged commit 4c40f75 into emqx30 Oct 18, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@spring2maz spring2maz deleted the Response&Request branch Oct 18, 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.