Add delays to SM buffer #4407
Add delays to SM buffer #4407
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4407 +/- ##
==========================================
- Coverage 85.32% 85.27% -0.05%
==========================================
Files 550 550
Lines 33905 33913 +8
==========================================
- Hits 28929 28919 -10
- Misses 4976 4994 +18 ☔ View full report in Codecov by Sentry. |
It is rerouted when no session was resumed and the resume_timeout has passed. This resulted in suprising messages, sometimes after a long time.
gustawlippa
force-pushed
the
fix-duplicated-mam-msg
branch
from
014ce65 to
53e2120
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This is allowed by the delayed delivery XEP. It should make it more clear where the delay is coming from for the user and in the case of testing/debugging.
This comment was marked as outdated.
This comment was marked as outdated.
The tests weren't really waiting for a timeout before. Extracted the tests into their own group and added one with different resource to show how it is handled by MIM.
gustawlippa
force-pushed
the
fix-duplicated-mam-msg
branch
from
5300f81 to
3072292
Compare
This comment was marked as outdated.
This comment was marked as outdated.
The initial presence response and delayed message/presence can come in any order.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| {_From, _To, El} = mongoose_acc:packet(AccWithTS), | ||
| El |
There was a problem hiding this comment.
mongoose_acc:element(Acc) will give you the element more cleanly than with packet.
There was a problem hiding this comment.
Ahh, thanks, let me change that.
There was a problem hiding this comment.
I amended the previous commit
gustawlippa
force-pushed
the
fix-duplicated-mam-msg
branch
from
e242184 to
b8d4427
Compare
gustawlippa
force-pushed
the
fix-duplicated-mam-msg
branch
from
b8d4427 to
4415dbe
Compare
|
elasticsearch_and_cassandra_27 / elasticsearch_and_cassandra_mnesia / 4415dbe small_tests_27 / small_tests / 4415dbe small_tests_27_arm64 / small_tests / 4415dbe small_tests_26 / small_tests / 4415dbe ldap_mnesia_26 / ldap_mnesia / 4415dbe ldap_mnesia_27 / ldap_mnesia / 4415dbe dynamic_domains_mysql_redis_27 / mysql_redis / 4415dbe internal_mnesia_27 / internal_mnesia / 4415dbe pgsql_cets_27 / pgsql_cets / 4415dbe dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 4415dbe dynamic_domains_mssql_mnesia_27 / odbc_mssql_mnesia / 4415dbe dynamic_domains_pgsql_mnesia_27 / pgsql_mnesia / 4415dbe pgsql_mnesia_26 / pgsql_mnesia / 4415dbe cockroachdb_cets_27 / cockroachdb_cets / 4415dbe graphql_session_SUITE:admin_session:admin_session_cli:admin_list_users_with_status{error,
{{assertEqual,
[{module,graphql_session_SUITE},
{line,591},
{expression,"lists : sort ( ActualJIDs )"},
{expected,
[<<"alice_admin_list_users_with_status_1224@localhost.bis/res1">>,
<<"alice_admin_list_users_with_status_1224@localhost/res1">>]},
{value,[]}]},
[{graphql_session_SUITE,check_users,2,
[{file,
"/home/circleci/project/big_tests/tests/graphql_session_SUITE.erl"},
{line,591}]},
{graphql_session_SUITE,admin_list_users_with_status_story,3,
[{file,
"/home/circleci/project/big_tests/tests/graphql_session_SUITE.erl"},
{line,416}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1794}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1303}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1235}]}]}}pubsub_SUITE:dag+manage_subscriptions:modify_node_subscriptions_test{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"alice_modify_node_subscriptions_test_2974@localhost/res1">>,
escalus_tcp,<0.77764.0>,
[{event_manager,<0.77740.0>},
{server,<<"localhost">>},
{username,<<"alicE_modify_node_subscriptions_test_2974">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.77740.0>},
{server,<<"localhost">>},
{username,
<<"alicE_modify_node_subscriptions_test_2974">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"alice_modify_node_subscriptions_test_2974">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"alicE_modify_node_subscriptions_test_2974">>},
{server,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"0e0061e568b5f10d">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{pubsub_tools,receive_response,3,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,433}]},
{pubsub_tools,receive_and_check_response,4,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"}...mysql_redis_27 / mysql_redis / 4415dbe pgsql_mnesia_27 / pgsql_mnesia / 4415dbe mssql_mnesia_27 / odbc_mssql_mnesia / 4415dbe cockroachdb_cets_27 / cockroachdb_cets / 4415dbe |
This PR adds
<delay>elements to stanzas rerouted throughmod_stream_management. This element was already present when a user requested stream resumption, and had unacknowledged stanzas.Currently, when a user reconnects, has unacknowledged stanzas, and doesn't request stream resumption, the stream management rerouting behaviour is to try to resend the stanzas to them, because MIM doesn't know if they have reached the user. However, if the user is using a different resource, the stanzas would get rerouted only after
resume_timeoutpasses, which could be surprising without the<delay>element. Even when rerouting to the same resource before the timeout, I think adding the<delay>could help client developers detect retransmitted stanzas.The
<delay>elements sent by SM andmod_presencewill now also include a description of where the delay is coming from (this is in allowed by https://xmpp.org/extensions/xep-0203.html). This may improve debugging in corner cases.I've also applied some fixes to the
sm_SUITEand reworked theresend_unacked_after_resume_timeoutso that the testcase reflects the title. With the new changes it is indeed checking theafter_resume_timeoutpart. I've added a similar case which checks the behaviour for users reconnecting with a different resource.