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 3 issues in SM resumption #2049

Merged
merged 5 commits into from
Mar 20, 2019
Merged

Fix 3 issues in SM resumption #2049

merged 5 commits into from
Mar 20, 2019

Conversation

fenek
Copy link
Member

@fenek fenek commented Aug 24, 2018

This PR addresses #2020

  1. Flushing messages during SM handover was done improperly

Function responsible for gathering incoming messages was returning them in wrong format (returned tuples had wrong elements).

  1. Presence subscription requests are buffered improperly

Subscription requests were resent with From and To as binaries, not JID record, leading to an error.

  1. Conflict check failed improperly after successful resumption

If a session sends a request and the reply is received by a new process (the resumed one), there is a SID mismatch. In order to deal with it, new C2S remembers old SID to use it for conflict check.

TODO:

  • Better description
  • Test for 1
  • Test for 2
  • Test for 3
  • Verify if from and to may be retrieved from Acc in ejabberd_c2s:resend_subscription_requests

Blocked until #2054 is merged, because it relies on module injection helper.

@mongoose-im

This comment has been minimized.

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the TODO list :) Also I had one question to the code.

@@ -2536,7 +2540,8 @@ resend_subscription_requests(Acc, #state{pending_invitations = Pending} = StateD
A1 = send_element(A, XMLPacket, State),
{value, From} = xml:get_tag_attr(<<"from">>, XMLPacket),
{value, To} = xml:get_tag_attr(<<"to">>, XMLPacket),
BufferedStateData = buffer_out_stanza({From, To, XMLPacket}, State),
PacketTuple = {jid:from_binary(From), jid:from_binary(To), XMLPacket},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have from and to in the Acc at this point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at the lines above - they are retrieved directly from xmlel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Maybe this question would be better. Are the from and to attributes the same in the stanza and in the Account?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, however this code seems to be correct as it is covered with tests. I'll add this item to TODO list. :)

@fenek fenek force-pushed the c2s-resumption-edge-cases branch from 36ebe3e to f51a925 Compare March 12, 2019 13:22
@fenek fenek force-pushed the c2s-resumption-edge-cases branch from f51a925 to 5aca772 Compare March 19, 2019 16:19
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 19, 2019

6264.1 / Erlang 19.3 / small_tests / 9c05df1
Reports root / small


6264.5 / Erlang 19.3 / ldap_mnesia / 9c05df1
Reports root/ big
OK: 1160 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0


6264.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 9c05df1
Reports root/ big
OK: 469 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0


6264.3 / Erlang 19.3 / mysql_redis / 9c05df1
Reports root/ big
OK: 3069 / Failed: 1 / User-skipped: 232 / Auto-skipped: 0

pubsub_SUITE:tree+hometree_specific:can_create_node_with_existing_parent_path
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,<<"alicE87.960954@localhost/res1">>,escalus_tcp,
          <0.11555.3>,
          [{event_manager,<0.11542.3>},
           {server,<<"localhost">>},
           {username,<<"alicE87.960954">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.11542.3>},
            {server,<<"localhost">>},
            {username,<<"alicE87.960954">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"alicE87.960954">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"alicE87.960954">>},
           {server,<<"localhost">>},
           {password,<<"matygrysa">>},
           {stream_id,<<"AC03FDB4B3D6D647">>}]},
        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,'-can_create_node_with_existing_parent_path/1-fun-0-',
       1,
       [{file,"pubsub_SUITE.erl"},{line,1767}]},
     {escalus_story,story,4,
       [{file,
          "/home/travis/build/esl/MongooseIM/...

Report log


6264.2 / Erlang 19.3 / internal_mnesia / 9c05df1
Reports root/ big
OK: 1195 / Failed: 0 / User-skipped: 65 / Auto-skipped: 0


6264.4 / Erlang 19.3 / odbc_mssql_mnesia / 9c05df1
Reports root/ big
OK: 3094 / Failed: 2 / User-skipped: 230 / 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,<<"eve98.481058@localhost/res1">>,escalus_tcp,
          <0.22179.3>,
          [{event_manager,<0.22170.3>},
           {server,<<"localhost">>},
           {username,<<"eve98.481058">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.22170.3>},
            {server,<<"localhost">>},
            {username,<<"eve98.481058">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"eve98.481058">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"eve98.481058">>},
           {server,<<"localhost">>},
           {password,<<"password">>},
           {port,5222},
           {stream_management,true},
           {stream_id,<<"FC349A65B5F92052">>}]},
        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}]},
    ...

Report log

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,<<"eve51.906236@localhost/res1">>,escalus_tcp,
          <0.23015.3>,
          [{event_manager,<0.23006.3>},
           {server,<<"localhost">>},
           {username,<<"eve51.906236">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.23006.3>},
            {server,<<"localhost">>},
            {username,<<"eve51.906236">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"eve51.906236">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"eve51.906236">>},
           {server,<<"localhost">>},
           {password,<<"password">>},
           {port,5222},
           {stream_management,true},
           {stream_id,<<"8B2724D2368F383C">>}]},
        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}]},
    ...

Report log


6264.8 / Erlang 20.0 / pgsql_mnesia / 9c05df1
Reports root/ big / small
OK: 3100 / Failed: 0 / User-skipped: 198 / Auto-skipped: 0


6264.9 / Erlang 21.0 / riak_mnesia / 9c05df1
Reports root/ big / small
OK: 1430 / Failed: 0 / User-skipped: 63 / Auto-skipped: 0


6264.4 / Erlang 19.3 / odbc_mssql_mnesia / 9c05df1
Reports root/ big
OK: 3068 / Failed: 0 / User-skipped: 230 / Auto-skipped: 0

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #2049 into master will increase coverage by 0.01%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2049      +/-   ##
==========================================
+ Coverage   78.77%   78.79%   +0.01%     
==========================================
  Files         333      333              
  Lines       28788    28793       +5     
==========================================
+ Hits        22679    22688       +9     
+ Misses       6109     6105       -4
Impacted Files Coverage Δ
src/jlib.erl 83.57% <100%> (+0.06%) ⬆️
src/ejabberd_c2s.erl 87.03% <87.5%> (-0.12%) ⬇️
src/cassandra/mongoose_cassandra_worker.erl 74.38% <0%> (-0.99%) ⬇️
src/pubsub/mod_pubsub.erl 71.76% <0%> (+0.06%) ⬆️
src/mod_roster.erl 80.09% <0%> (+0.23%) ⬆️
src/ejabberd_router.erl 74.21% <0%> (+1.25%) ⬆️
src/pubsub/nodetree_tree.erl 90.9% <0%> (+3.03%) ⬆️
src/global_distrib/mod_global_distrib_receiver.erl 89.47% <0%> (+3.94%) ⬆️

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 bc5e861...5aca772. Read the comment docs.

Copy link
Contributor

@aleklisi aleklisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@aleklisi aleklisi merged commit 552a91c into master Mar 20, 2019
@erszcz erszcz deleted the c2s-resumption-edge-cases branch March 20, 2019 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants