Wait longer for replaced c2s #2054

fenek · 2018-08-29T12:51:03Z

This PR addresses #1880

When a node is slow, it may take more than 100ms for the replaced session to close or for 'DOWN' message to reach the new c2s.

TODO:

Tests
C2S state could store replaced PIDs list in the state so they may be ignored in handle_info
Wait time for replaced sessions may be configurable
Docs

codecov · 2018-08-29T13:58:14Z

Codecov Report

Merging #2054 into master will decrease coverage by <.01%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #2054      +/-   ##
==========================================
- Coverage   78.86%   78.85%   -0.01%     
==========================================
  Files         333      333              
  Lines       28777    28788      +11     
==========================================
+ Hits        22694    22701       +7     
- Misses       6083     6087       +4

Impacted Files	Coverage Δ
src/ejabberd_c2s.erl	`87.14% <94.73%> (+0.26%)`	⬆️
src/mam/mod_mam_rdbms_prefs.erl	`92.52% <0%> (-3.74%)`	⬇️
src/rdbms/mongoose_rdbms.erl	`68.87% <0%> (-2.56%)`	⬇️
src/mam/mod_mam_utils.erl	`82.78% <0%> (-0.67%)`	⬇️
src/mod_muc_log.erl	`77.69% <0%> (-0.52%)`	⬇️
src/pubsub/mod_pubsub.erl	`71.7% <0%> (-0.49%)`	⬇️
src/mod_bosh_socket.erl	`82.17% <0%> (+0.33%)`	⬆️
src/mod_muc_room.erl	`77.5% <0%> (+0.46%)`	⬆️
src/ejabberd_router.erl	`74.21% <0%> (+1.25%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9939861...a436d8d. Read the comment docs.

src/ejabberd_c2s.erl

big_tests/tests/connect_SUITE.erl

src/ejabberd_c2s.erl

big_tests/tests/connect_SUITE.erl

erszcz · 2019-03-07T10:56:05Z

Awesome! I think connection replacement and resumption is one of the pain points for client app developers - really good stuff!

src/ejabberd_c2s.erl

goddammit · 2019-03-07T15:14:28Z

doc/Advanced-configuration.md

@@ -328,6 +328,11 @@ There are some additional options that influence all database connections in the
    * **Default:** `[mongoose_router_global, mongoose_router_localdomain, mongoose_router_external_localnode, mongoose_router_external, ejabberd_s2s]`
    * **Example:** `{routing_modules, [mongoose_router_global, mongoose_router_localdomain]}.`

+* **replaced_wait_timeout** (local)
+    * **Description:** When a user session is replaced (due to a full JID conflict) by a new one, this parameter specifies time that MongooseIM waits for old sessions to close. The default value is sufficient in most cases. If you observe `replaced_wait_timeout` warning in logs, then most probably old sessions are frozen for some reason and it should be investigated.


Description: When a user session is replaced (due to a full JID conflict) by a new one, this parameter specifies the time MongooseIM waits for the old sessions to close. The default value is sufficient in most cases. If you observe replaced_wait_timeout warning in logs, then most probably rgw old sessions are frozen for some reason and it should be investigated.

kanes115 · 2019-03-08T14:02:45Z

big_tests/tests/mongoose_helper.erl

@@ -210,9 +212,13 @@ forget_persistent_rooms() ->
    rpc(mim(), mnesia, clear_table, [muc_registered]),
    ok.

-spec successful_rpc(atom(), atom(), list()) -> term().
+-spec successful_rpc(M :: atom(), F :: atom(), A :: list()) -> term().


Does MFA even need typespecs? :D

There are even builtin mfa/0 and module/0 types.

kanes115 · 2019-03-08T14:07:07Z

big_tests/tests/mongoose_helper.erl

+inject_module(Module) ->
+    inject_module(Module, false).
+
+-spec inject_module(Module :: module(), CheckIfLoadedAlready :: boolean()) -> ok | already_loaded.


I feel like DoOverwrite would better explain what is its role

OK, will pick a better name

Boolean params are evil. If you don't know the definition by heart, what does this call site tell you?

inject_module(Module, false)

Inject if true, do not if false - so do not really inject the module here? Inject, but false what?
Instead: inject_module(Module, force_reload), or inject_module(Module, do_overwrite), or inject_module(Module, [force_reload])...

kanes115 · 2019-03-08T14:17:57Z

src/ejabberd_c2s.erl

+            ok;
+        _ ->
+            Timeout = get_replaced_wait_timeout(S),
+            erlang:send_after(Timeout, self(), replaced_wait_timeout)


I am not sure whether it is crucial, but maybe it would be worth to use TRef? In order to prevent c2s process from getting messages replaced_wait_timeout from different sources than itself?

Is there some another source of such message? Not really :)

mongoose-im · 2019-03-18T12:25:44Z

6257.1 / Erlang 19.3 / small_tests / 2c08eb3
Reports root / small

mongoose-im · 2019-03-18T13:51:47Z

6258.1 / Erlang 19.3 / small_tests / 54ce9a8
Reports root / small

6258.5 / Erlang 19.3 / ldap_mnesia / 54ce9a8
Reports root/ big
OK: 1157 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0

6258.2 / Erlang 19.3 / internal_mnesia / 54ce9a8
Reports root/ big
OK: 1192 / Failed: 0 / User-skipped: 65 / Auto-skipped: 0

6258.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 54ce9a8
Reports root/ big
OK: 469 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0

6258.3 / Erlang 19.3 / mysql_redis / 54ce9a8
Reports root/ big
OK: 3063 / Failed: 0 / User-skipped: 232 / Auto-skipped: 0

6258.4 / Erlang 19.3 / odbc_mssql_mnesia / 54ce9a8
Reports root/ big
OK: 3065 / Failed: 0 / User-skipped: 230 / Auto-skipped: 0

6258.8 / Erlang 20.0 / pgsql_mnesia / 54ce9a8
Reports root/ big / small
OK: 3097 / Failed: 0 / User-skipped: 198 / Auto-skipped: 0

6258.9 / Erlang 21.0 / riak_mnesia / 54ce9a8
Reports root/ big / small
OK: 1440 / Failed: 1 / User-skipped: 63 / Auto-skipped: 0

mod_global_distrib_SUITE:mod_global_distrib:test_pm_with_ungraceful_reconnection_to_different_server

{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,<<"eve90.56542@localhost/res1">>,escalus_tcp,
          <0.25627.1>,
          [{event_manager,<0.25618.1>},
           {server,<<"localhost">>},
           {username,<<"eve90.56542">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.25618.1>},
            {server,<<"localhost">>},
            {username,<<"eve90.56542">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"eve90.56542">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"eve90.56542">>},
           {server,<<"localhost">>},
           {password,<<"password">>},
           {port,5222},
           {stream_management,true},
           {stream_id,<<"0629346B1ABA6CDE">>}]},
        10000],
       [{file,
          "/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,138}]},
     {mod_global_distrib_SUITE,
       '-test_pm_with_ungraceful_reconnection_to_different_server/1-fun-0-',
       4,
       [{file,"mod_global_distrib_SUITE.erl"},{line,610}]},
     {escalus_story,story,4,
       [{file,
          "/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
        {line,72}]},
     {tes...

Report log

6258.1 / Erlang 19.3 / small_tests / 54ce9a8
Reports root / small

6258.2 / Erlang 19.3 / internal_mnesia / 54ce9a8
Reports root/ big
OK: 1205 / Failed: 1 / User-skipped: 65 / Auto-skipped: 0

pubsub_SUITE:dag+collection:notify_collection_leaf_and_item_test

{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,<<"bOb81.274969@localhost/res1">>,escalus_tcp,
          <0.4318.1>,
          [{event_manager,<0.4255.1>},
           {server,<<"localhost">>},
           {username,<<"bOb81.274969">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.4255.1>},
            {server,<<"localhost">>},
            {username,<<"bOb81.274969">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"bOb81.274969">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"bOb81.274969">>},
           {server,<<"localhost">>},
           {password,<<"makrolika">>},
           {stream_id,<<"6CA1E0ADC05383E1">>}]},
        5000],
       [{file,
          "/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,138}]},
     {pubsub_tools,receive_notification,3,
       [{file,"pubsub_tools.erl"},{line,480}]},
     {pubsub_tools,receive_node_creation_notification,3,
       [{file,"pubsub_tools.erl"},{line,304}]},
     {pubsub_SUITE,'-notify_collection_leaf_and_item_test/1-fun-0-',2,
       [{file,"pubsub_SUITE.erl"},{line,1266}]},
     {escalus_story,story,4,
       [{file,
          "/home/travis/build/esl/MongooseIM/big_tests/_bui...

Report log

6258.3 / Erlang 19.3 / mysql_redis / 54ce9a8
Reports root/ big
OK: 3076 / Failed: 1 / User-skipped: 232 / Auto-skipped: 0

pubsub_SUITE:dag+collection:discover_child_nodes_test

{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,<<"alicE86.623382@localhost/res1">>,escalus_tcp,
          <0.10178.3>,
          [{event_manager,<0.10126.3>},
           {server,<<"localhost">>},
           {username,<<"alicE86.623382">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.10126.3>},
            {server,<<"localhost">>},
            {username,<<"alicE86.623382">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"alicE86.623382">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"alicE86.623382">>},
           {server,<<"localhost">>},
           {password,<<"matygrysa">>},
           {stream_id,<<"4181867406491172">>}]},
        5000],
       [{file,
          "/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,138}]},
     {pubsub_tools,receive_response,3,
       [{file,"pubsub_tools.erl"},{line,456}]},
     {pubsub_tools,receive_and_check_response,4,
       [{file,"pubsub_tools.erl"},{line,447}]},
     {pubsub_SUITE,'-discover_child_nodes_test/1-fun-0-',2,
       [{file,"pubsub_SUITE.erl"},{line,1493}]},
     {escalus_story,story,4,
       [{file,
          "/home/travis/build/esl/MongooseIM/big_tests/_build/default...

Report log

6258.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 54ce9a8
Reports root/ big
OK: 469 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0

6258.5 / Erlang 19.3 / ldap_mnesia / 54ce9a8
Reports root/ big
OK: 1157 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0

6258.4 / Erlang 19.3 / odbc_mssql_mnesia / 54ce9a8
Reports root/ big
OK: 3065 / Failed: 0 / User-skipped: 230 / Auto-skipped: 0

6258.8 / Erlang 20.0 / pgsql_mnesia / 54ce9a8
Reports root/ big / small
OK: 3097 / Failed: 0 / User-skipped: 198 / Auto-skipped: 0

6258.9 / Erlang 21.0 / riak_mnesia / 54ce9a8
Reports root/ big / small
OK: 1427 / Failed: 0 / User-skipped: 63 / Auto-skipped: 0

fenek · 2019-03-18T16:39:36Z

Succeeded on Travis twice without restarts.

aleklisi

PR looks good 👍 I like the src/ejabberd_c2s.erl refactor :)

big_tests/tests/connect_SUITE.erl

aleklisi · 2019-03-19T08:04:04Z

big_tests/tests/connect_SUITE.erl

-     ,{group, just_tls}
+    [
+     {group, session_replacement},
+     %% these groups must be last, as they really... complicate configuration


Could you elaborate?

aleklisi · 2019-03-19T08:19:55Z

big_tests/tests/connect_SUITE.erl

+    UserSpec = [{resource, <<"conflict">>} | escalus_users:get_userspec(Config, alice)],
+    {ok, Alice1, _} = escalus_connection:start(UserSpec),
+
+    {ok, Alice2, _} = escalus_connection:start(UserSpec),


Could you please rename
Alice1 -> Alice and Alice2 -> Bellatrix
Alice1 -> Geralt and Alice2 -> Ciri
or something like that?
;)

Not a good idea. Different names are used for different users. Different resources of the same user are suffixed, usually by a number. This is the convention used across all big tests.

aleklisi · 2019-03-19T08:25:31Z

big_tests/tests/connect_SUITE.erl

+    {ok, Alice2, _} = escalus_connection:start(UserSpec),
+
+    % THEN a timeout warning is logged
+    FilterFun = fun(_, Msg) ->


Consider extracting this lambda to named private method because you use it in clean_close_of_replaced_session/1 and replaced_session_cannot_terminate/1 ??

aleklisi · 2019-03-19T08:29:02Z

big_tests/tests/lager_ct_backend.erl

+
+-import(mongoose_helper, [successful_rpc/3]).
+
+-type state() :: #{


If this map contains one key only, why not directly use list (value for only key) as a state instead?

Something like:

-type state() :: [{pid(), lager:log_level()}].

It's less refactoring when you actually need to add one more field, which often happens with state.

doc/Advanced-configuration.md

erszcz · 2019-03-19T11:37:54Z

Awesome!

fenek added WIP 🚧 waiting-for-ci labels Aug 29, 2018

This comment has been minimized.

Sign in to view

fenek added 3 commits March 5, 2019 12:20

Wait longer for replaced c2s

0b04f80

Add smart handling of replaced c2s that timed out

fb0217b

Refactor code injection in big tests

988b816

fenek force-pushed the fix-down-error branch from 1e31c3d to 988b816 Compare March 7, 2019 09:59

fenek commented Mar 7, 2019

View reviewed changes

src/ejabberd_c2s.erl Outdated Show resolved Hide resolved

Small c2s refactor

54b7c9e

This comment has been minimized.

Sign in to view

Add replaced_wait_timeout opt to docs

eb25fec

fenek added waiting-for-review and removed WIP 🚧 labels Mar 7, 2019

erszcz reviewed Mar 7, 2019

View reviewed changes

big_tests/tests/connect_SUITE.erl Outdated Show resolved Hide resolved

erszcz reviewed Mar 7, 2019

View reviewed changes

big_tests/tests/connect_SUITE.erl Outdated Show resolved Hide resolved

erszcz reviewed Mar 7, 2019

View reviewed changes

src/ejabberd_c2s.erl Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

erszcz reviewed Mar 7, 2019

View reviewed changes

big_tests/tests/connect_SUITE.erl Show resolved Hide resolved

erszcz reviewed Mar 7, 2019

View reviewed changes

big_tests/tests/connect_SUITE.erl Outdated Show resolved Hide resolved

erszcz reviewed Mar 7, 2019

View reviewed changes

big_tests/tests/connect_SUITE.erl Outdated Show resolved Hide resolved

Add missing export to mongoose_helper

acf41be

erszcz reviewed Mar 7, 2019

View reviewed changes

src/ejabberd_c2s.erl Show resolved Hide resolved

fenek added 3 commits March 7, 2019 12:20

Fix code injection in global_distrib suite

814f48b

Apply review suggestions

7ff4166

Fix missing exports again

9d75d6d

This comment has been minimized.

Sign in to view

Add one more fix for session replacement tests

7e11e81

This comment has been minimized.

Sign in to view

goddammit reviewed Mar 7, 2019

View reviewed changes

erszcz approved these changes Mar 7, 2019

View reviewed changes

Better and working session replacement tests

5f90789

This comment has been minimized.

Sign in to view

kanes115 self-requested a review March 8, 2019 14:00

kanes115 reviewed Mar 8, 2019

View reviewed changes

fenek mentioned this pull request Mar 12, 2019

Fix 3 issues in SM resumption #2049

Merged

5 tasks

fenek added 2 commits March 12, 2019 14:54

Improve names in big test injection helper API

7c20265

Apply language review

ccdf1b3

This comment has been minimized.

Sign in to view

Stop clients properly in session replacement tests

97699f1

This comment has been minimized.

Sign in to view

Fix race condition in sm_SUITE

d817fc6

This comment has been minimized.

Sign in to view

fenek mentioned this pull request Mar 15, 2019

Fix down error 2 #2239

Closed

fenek force-pushed the fix-down-error branch from a0b1fca to 59f2f5d Compare March 18, 2019 12:12

Add proper wait for c2s state in sm_SUITE

a436d8d

fenek force-pushed the fix-down-error branch from 59f2f5d to a436d8d Compare March 18, 2019 13:38

fenek removed the waiting-for-ci label Mar 18, 2019

aleklisi reviewed Mar 19, 2019

View reviewed changes

erszcz merged commit bc5e861 into master Mar 19, 2019

erszcz deleted the fix-down-error branch March 19, 2019 11:37

fenek added this to the MongooseIM 3.3.0++ milestone Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait longer for replaced c2s #2054

Wait longer for replaced c2s #2054

fenek commented Aug 29, 2018 •

edited

Loading

This comment has been minimized.

codecov bot commented Aug 29, 2018 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

erszcz commented Mar 7, 2019

This comment has been minimized.

This comment has been minimized.

goddammit Mar 7, 2019

erszcz Mar 7, 2019

This comment has been minimized.

kanes115 Mar 8, 2019

erszcz Mar 8, 2019

kanes115 Mar 8, 2019

fenek Mar 11, 2019

erszcz Mar 11, 2019

kanes115 Mar 8, 2019

fenek Mar 11, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mongoose-im commented Mar 18, 2019

mongoose-im commented Mar 18, 2019 •

edited

Loading

fenek commented Mar 18, 2019

aleklisi left a comment

aleklisi Mar 19, 2019

aleklisi Mar 19, 2019

erszcz Mar 19, 2019 •

edited

Loading

aleklisi Mar 19, 2019

aleklisi Mar 19, 2019

aleklisi Mar 19, 2019

erszcz Mar 19, 2019

erszcz commented Mar 19, 2019


		-import(mongoose_helper, [successful_rpc/3]).

		-type state() :: #{

Wait longer for replaced c2s #2054

Wait longer for replaced c2s #2054

Conversation

fenek commented Aug 29, 2018 • edited Loading

This comment has been minimized.

codecov bot commented Aug 29, 2018 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

erszcz commented Mar 7, 2019

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mongoose-im commented Mar 18, 2019

mongoose-im commented Mar 18, 2019 • edited Loading

fenek commented Mar 18, 2019

aleklisi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erszcz Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erszcz commented Mar 19, 2019

fenek commented Aug 29, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading

mongoose-im commented Mar 18, 2019 •

edited

Loading

erszcz Mar 19, 2019 •

edited

Loading