Merge pull request #4501 from esl/fix-sm-h-counting

Fix SM counting

Author: chrzaszcz

big_tests/tests/sm_SUITE.erl

@@ -103,6 +103,9 @@ parallel_cases() ->
      h_ok_after_session_enabled_before_session,
      h_ok_after_session_enabled_after_session,
      h_ok_after_a_chat,
+     h_ok_after_presence,
+     h_ok_after_iq,
+     h_ok_after_non_xmpp_stanza,
      h_non_given_closes_stream_gracefully,
      resend_unacked_on_reconnection,
      session_established,
@@ -401,18 +404,14 @@ basic_ack(Config) ->
 %% - <r/> is sent *before* the session is established
 h_ok_before_session(Config) ->
     User = connect_fresh(Config, ?config(user, Config), sm_after_bind),
-    escalus_connection:send(User, escalus_stanza:sm_request()),
-    escalus:assert(is_sm_ack, [0],
-                   escalus_connection:get_stanza(User, stream_mgmt_ack)).
+    assert_h(User, 0).
 
 %% Test that "h" value is valid when:
 %% - SM is enabled *before* the session is established
 %% - <r/> is sent *after* the session is established
 h_ok_after_session_enabled_before_session(Config) ->
     User = connect_fresh(Config, ?config(user, Config), sm_before_session),
-    escalus_connection:send(User, escalus_stanza:sm_request()),
-    escalus:assert(is_sm_ack, [1],
-                   escalus_connection:get_stanza(User, stream_mgmt_ack)).
+    assert_h(User, 1).
 
 %% Test that "h" value is valid when:
 %% - SM is enabled *after* the session is established
@@ -422,15 +421,14 @@ h_ok_after_session_enabled_after_session(Config) ->
     escalus_connection:send(User, escalus_stanza:roster_get()),
     escalus:assert(is_roster_result,
                    escalus_connection:get_stanza(User, roster_result)),
-    escalus_connection:send(User, escalus_stanza:sm_request()),
-    escalus:assert(is_sm_ack, [1],
-                   escalus_connection:get_stanza(User, stream_mgmt_ack)).
+    assert_h(User, 1).
 
 %% Test that "h" value is valid after exchanging a few messages.
 h_ok_after_a_chat(ConfigIn) ->
     Config = escalus_users:update_userspec(ConfigIn, ?config(user, ConfigIn),
                                            stream_management, true),
     escalus:fresh_story(Config, [{?config(user, Config), 1}, {bob,1}], fun(User, Bob) ->
+        assert_h(User, 1),
         escalus:send(User, escalus_stanza:chat_to(Bob, <<"Hi, Bob!">>)),
         escalus:assert(is_chat_message, [<<"Hi, Bob!">>],
                        escalus:wait_for_stanza(Bob)),
@@ -443,12 +441,47 @@ h_ok_after_a_chat(ConfigIn) ->
         escalus:send(User, escalus_stanza:chat_to(Bob, <<"Pretty !@#$%^$">>)),
         escalus:assert(is_chat_message, [<<"Pretty !@#$%^$">>],
                        escalus:wait_for_stanza(Bob)),
-        escalus:send(User, escalus_stanza:sm_request()),
-        escalus:assert(is_sm_ack, [3], escalus:wait_for_stanza(User)),
+        assert_h(User, 3),
         %% Ack, so that unacked messages don't go into offline store.
         escalus:send(User, escalus_stanza:sm_ack(3))
     end).
 
+h_ok_after_presence(Config) ->
+    User = connect_fresh(Config, ?config(user, Config), sm_before_session),
+    assert_h(User, 1),
+    Presence = escalus_stanza:presence(<<"available">>),
+    escalus:send(User, Presence),
+    escalus:assert(is_presence, escalus:wait_for_stanza(User)),
+    assert_h(User, 2),
+    escalus:send(User, Presence),
+    escalus:assert(is_presence, escalus:wait_for_stanza(User)),
+    assert_h(User, 3).
+
+h_ok_after_iq(Config) ->
+    User = connect_fresh(Config, ?config(user, Config), sm_before_session),
+    assert_h(User, 1),
+    Iq = escalus_stanza:iq_get(<<"invalid_ns">>, []),
+    escalus_client:send(User, Iq),
+    escalus:assert(is_iq_error, escalus:wait_for_stanza(User)),
+    assert_h(User, 2),
+    escalus_client:send(User, Iq),
+    escalus:assert(is_iq_error, escalus:wait_for_stanza(User)),
+    assert_h(User, 3).
+
+h_ok_after_non_xmpp_stanza(Config) ->
+    User = connect_fresh(Config, ?config(user, Config), sm_before_session),
+    assert_h(User, 1),
+    %% SM stanzas are not counted
+    assert_h(User, 1),
+    %% CSI stanzas are not counted
+    CsiActive = csi_helper:csi_stanza(<<"active">>),
+    escalus_client:send(User, CsiActive),
+    assert_h(User, 1),
+    %% any non-xmpp stanza is not counted
+    Stanza =  #xmlel{name = <<"dummy_stanza">>},
+    escalus_client:send(User, Stanza),
+    assert_h(User, 1).
+
 h_non_given_closes_stream_gracefully(ConfigIn) ->
     AStanza = #xmlel{name = <<"a">>,
                      attrs = #{<<"xmlns">> => <<"urn:xmpp:sm:3">>}},
@@ -1601,3 +1634,7 @@ maybe_ack_initial_presence(User, ack) ->
     ack_initial_presence(User);
 maybe_ack_initial_presence(_User, no_ack) ->
     ok.
+
+assert_h(User, H) ->
+    escalus:send(User, escalus_stanza:sm_request()),
+    escalus:assert(is_sm_ack, [H], escalus:wait_for_stanza(User)).

src/stream_management/mod_stream_management.erl

@@ -178,8 +178,8 @@ stale_h_config_spec() ->
 -spec user_send_packet(mongoose_acc:t(), mongoose_c2s_hooks:params(), gen_hook:extra()) ->
     mongoose_c2s_hooks:result().
 user_send_packet(Acc, #{c2s_data := StateData}, _Extra) ->
-    case {get_mod_state(StateData), is_sm_element(Acc)} of
-        {#sm_state{counter_in = Counter} = SmState, false} ->
+    case {get_mod_state(StateData), is_stanza_element(Acc)} of
+        {#sm_state{counter_in = Counter} = SmState, true} ->
             NewSmState = SmState#sm_state{counter_in = incr_counter(Counter)},
             {ok, mongoose_c2s_acc:to_acc(Acc, state_mod, {?MODULE, NewSmState})};
         {_, _} ->
@@ -856,7 +856,7 @@ sm_state_to_keep(SmState, From) ->
 recover_messages(SmState) ->
     receive
         {route, Acc} ->
-            recover_messages(maybe_buffer_acc(SmState, Acc, is_message(mongoose_acc:stanza_name(Acc))))
+            recover_messages(maybe_buffer_acc(SmState, Acc, is_message(Acc)))
     after 0 ->
               SmState
     end.
@@ -868,14 +868,23 @@ maybe_buffer_acc(SmState, _Acc, false) ->
     SmState.
 
 %% IQs and presences are allowed to come to the same SID only
--spec is_message(binary()) -> boolean().
-is_message(<<"message">>) -> true;
-is_message(_) -> false.
+-spec is_message(mongoose_acc:t()) -> boolean().
+is_message(Acc) ->
+    case mongoose_acc:stanza_name(Acc) of
+        <<"message">> -> true;
+        _  -> false
+    end.
 
--spec is_sm_element(mongoose_acc:t()) -> boolean().
-is_sm_element(Acc) ->
-    El = mongoose_acc:element(Acc),
-    ?NS_STREAM_MGNT_3 =:= exml_query:attr(El, <<"xmlns">>).
+%% XEP-0198 states that only XMPP stanzas (i.e. <iq/>, <message/> or <presence/>
+%% stanzas as defined in RFC 6120) should be counted or acked in stream management
+-spec is_stanza_element(mongoose_acc:t()) -> boolean().
+is_stanza_element(Acc) ->
+    case mongoose_acc:stanza_name(Acc) of
+        <<"message">> -> true;
+        <<"iq">> -> true;
+        <<"presence">> -> true;
+        _ -> false
+    end.
 
 -spec make_smid() -> smid().
 make_smid() ->