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

C2S/Enable pep_SUITE #3931

Merged
merged 7 commits into from
Jan 23, 2023
Merged

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jan 12, 2023

The goal is to enable PEP tests for mongoose_c2s. This required implementing missing parts of pubsub and caps, that were present directly in the legacy c2s implementation.

Functional changes:

  • PubSub is now using foreign_event and gen_statem:call to call the functionality that requires c2s state data. The PEP-related function are still implemented as hooks, because mod_caps shouldn't be called directly from another module. As little as possible is delegated to the c2s processes.
  • Added timeouts to gen_statem calls, because they were deadlock traps.

Test changes:

  • PEP Tests don't use timer:sleep and shouldn't fail randomly anymore. Some functionality is fixed, and remaining race conditions are handled explicitly.
  • Tests were performed with a repetition of 100 times to make sure that they are not flaky anymore.

Left for the future - out of scope for now:

  • Adding an API to mongoose_c2s to prevent calling gen_statem with infinite timeout.
  • Elimination of gen_statem calls in favor of requests. Alternatively, mod_caps could use mnesia table, even the session one, because the cached caps are propagated with presences, which makes them often inconsistent, and causes race conditions and unexpected behaviour (described in tests).
  • Possible usage of presence subscriptions in conjunction with filtering in the recipient's process to avoid checking the caps entirely.
  • Further optimizations of event sending to prevent the risk of duplication.

Follow-up stories can be groomed.

@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Base: 82.11% // Head: 82.67% // Increases project coverage by +0.56% 🎉

Coverage data is based on head (05322df) compared to base (1424476).
Patch coverage: 86.95% of modified lines in pull request are covered.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/mongoose_c2s    #3931      +/-   ##
========================================================
+ Coverage                 82.11%   82.67%   +0.56%     
========================================================
  Files                       540      540              
  Lines                     34198    34228      +30     
========================================================
+ Hits                      28082    28299     +217     
+ Misses                     6116     5929     -187     
Impacted Files Coverage Δ
src/mod_caps.erl 77.15% <76.92%> (+30.54%) ⬆️
src/mod_presence.erl 85.75% <85.71%> (-0.05%) ⬇️
src/pubsub/mod_pubsub.erl 73.37% <88.37%> (+5.96%) ⬆️
src/mongoose_hooks.erl 95.86% <100.00%> (+2.32%) ⬆️
src/pubsub/mod_pubsub_db.erl 57.14% <0.00%> (-28.58%) ⬇️
src/domain/mongoose_domain_loader.erl 89.28% <0.00%> (-0.90%) ⬇️
src/pubsub/mod_pubsub_db_mnesia.erl 92.40% <0.00%> (-0.85%) ⬇️
src/mod_roster.erl 78.65% <0.00%> (+0.23%) ⬆️
src/pubsub/mod_pubsub_db_rdbms.erl 95.34% <0.00%> (+0.77%) ⬆️
src/rdbms/mongoose_rdbms.erl 66.31% <0.00%> (+1.05%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the c2s/enable-remaining-suites branch 2 times, most recently from d014a04 to 77a33a9 Compare January 18, 2023 15:38
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

mongoose_c2s was missing the logic for handling PEP messages, because
now the logic does not belong in the c2s module anymore

Main changes:
- During send_last_pep_items, filtering by features is made on the spot
- Functionality is not delegated to the c2s process anymore.
- foreign_event is used to call the PEP-related hooks
- dispatch_items no longer delegates the sending to the c2s process.
  Now the process is only called to get/filter recipients by caps.
- unsubscribe_user is not spawning new processes anymore, because this
  behaviour was causing race conditions. The operation does not seem
  to be heavy.
@mongoose-im

This comment was marked as outdated.

One hook needed updating, another one was missing entirely.
Also: handle the case of IQ request timeout. It was crashing.
Also: add gen_statem call timeouts to prevent infinite deadlocks.
- Enable cache_tests, which were somehow disabled
- Handle the new stanza order, where the presence comes before the iq
  request.
- Remove arbitrary sleep's that were not fixing the race conditions
  anyway. Some race conditions still apply - and they are handled
  explicitly now.
- Test the newly discovered issue of missing notifications in case of
  one-way presence subscriptions.
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz changed the title C2S/Enable remaining suites C2S/Enable pep_SUITE Jan 20, 2023
@mongoose-im

This comment was marked as outdated.

Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Just a first first comment for now, more review in progress.

src/mongoose_hooks.erl Outdated Show resolved Hide resolved
@chrzaszcz chrzaszcz marked this pull request as ready for review January 20, 2023 16:31
Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

A couple more comments. I still need to read in depth the changes in the test suites, will do later today, but for now a few more notes :)

src/mod_caps.erl Outdated Show resolved Hide resolved
src/mod_caps.erl Outdated Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 23, 2023

small_tests_24 / small_tests / 05322df
Reports root / small


small_tests_25 / small_tests / 05322df
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 05322df
Reports root/ big
OK: 2157 / Failed: 0 / User-skipped: 803 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 05322df
Reports root/ big
OK: 4163 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 05322df
Reports root/ big
OK: 2157 / Failed: 0 / User-skipped: 803 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 05322df
Reports root/ big
OK: 4136 / Failed: 1 / User-skipped: 114 / Auto-skipped: 0

muc_SUITE:hibernation:hibernated_room_can_be_queried_for_archive
{error,{{assertion_failed,assert,is_groupchat_message,
              [<<"Restorable message">>],
              undefined,"undefined"},
    [{escalus_new_assert,assert_true,2,
               [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
                {line,84}]},
     {muc_SUITE,wait_for_mam_result,3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4394}]},
     {muc_SUITE,'-hibernated_room_can_be_queried_for_archive/1-fun-0-',3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4130}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {muc_SUITE,hibernated_room_can_be_queried_for_archive,1,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4126}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1291}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1223}]}]}}

Report log


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 05322df
Reports root/ big
OK: 4163 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 05322df
Reports root/ big
OK: 4465 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 05322df
Reports root/ big
OK: 4158 / Failed: 5 / User-skipped: 88 / Auto-skipped: 0

inbox_extensions_SUITE:regular:one_to_one:pagination:can_paginate_backwards
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,
          <<"[email protected]/res1">>,
          escalus_tcp,<0.12982.1>,
          [{event_manager,<0.12966.1>},
           {server,<<"domain.example.com">>},
           {username,<<"bOb_can_paginate_backwards_1503">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.12966.1>},
            {server,<<"domain.example.com">>},
            {username,<<"bOb_can_paginate_backwards_1503">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"bob_can_paginate_backwards_1503">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"bOb_can_paginate_backwards_1503">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {password,<<"makrolika">>},
           {stream_id,<<"af92a003a053c140">>}]},
        5000],
       [{file,
          "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,136}]},
     {inbox_helper,'-given_conversations_between/2-fun-1-',4,
       [{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
        {line,543}]},
     {lists,foldl,3,[{file,"lists.erl"},{line,1350}]},
     {inbox_extensions_SUITE...

Report log

inbox_extensions_SUITE:regular:one_to_one:pagination:can_paginate_forwards
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,
          <<"[email protected]/res1">>,
          escalus_tcp,<0.12983.1>,
          [{event_manager,<0.12931.1>},
           {server,<<"domain.example.com">>},
           {username,<<"kate_can_paginate_forwards_1498">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.12931.1>},
            {server,<<"domain.example.com">>},
            {username,<<"kate_can_paginate_forwards_1498">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"kate_can_paginate_forwards_1498">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"kate_can_paginate_forwards_1498">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {password,<<"makrowe;p">>},
           {stream_id,<<"34481132be694b12">>}]},
        5000],
       [{file,
          "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,136}]},
     {inbox_helper,'-given_conversations_between/2-fun-1-',4,
       [{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
        {line,543}]},
     {lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
     {inbox_extensions_SUI...

Report log

inbox_extensions_SUITE:regular:one_to_one:pagination:pagination_overrides_form
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,
          <<"[email protected]/res1">>,
          escalus_tcp,<0.12985.1>,
          [{event_manager,<0.12945.1>},
           {server,<<"domain.example.com">>},
           {username,<<"kate_pagination_overrides_form_1500">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.12945.1>},
            {server,<<"domain.example.com">>},
            {username,<<"kate_pagination_overrides_form_1500">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"kate_pagination_overrides_form_1500">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"kate_pagination_overrides_form_1500">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {password,<<"makrowe;p">>},
           {stream_id,<<"241a2cca05d1e2c2">>}]},
        5000],
       [{file,
          "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,136}]},
     {inbox_helper,'-given_conversations_between/2-fun-1-',4,
       [{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
        {line,543}]},
     {lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
     {...

Report log

inbox_extensions_SUITE:regular:one_to_one:pagination:max_queries_can_fetch_ahead
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,
          <<"[email protected]/res1">>,
          escalus_tcp,<0.12987.1>,
          [{event_manager,<0.12959.1>},
           {server,<<"domain.example.com">>},
           {username,<<"kate_max_queries_can_fetch_ahead_1502">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.12959.1>},
            {server,<<"domain.example.com">>},
            {username,<<"kate_max_queries_can_fetch_ahead_1502">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"kate_max_queries_can_fetch_ahead_1502">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"kate_max_queries_can_fetch_ahead_1502">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {password,<<"makrowe;p">>},
           {stream_id,<<"37a839c8a32df4e9">>}]},
        5000],
       [{file,
          "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,136}]},
     {inbox_helper,'-given_conversations_between/2-fun-1-',4,
       [{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
        {line,543}]},
     {lists,foldl_1,3,[{file,"lists.erl"},{line,1355}...

Report log

inbox_extensions_SUITE:regular:one_to_one:pagination:max_queries_can_be_limited
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,
          <<"[email protected]/res1">>,
          escalus_tcp,<0.12984.1>,
          [{event_manager,<0.12952.1>},
           {server,<<"domain.example.com">>},
           {username,<<"kate_max_queries_can_be_limited_1501">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.12952.1>},
            {server,<<"domain.example.com">>},
            {username,<<"kate_max_queries_can_be_limited_1501">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,<<"kate_max_queries_can_be_limited_1501">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,<<"kate_max_queries_can_be_limited_1501">>},
           {server,<<"domain.example.com">>},
           {host,<<"localhost">>},
           {password,<<"makrowe;p">>},
           {stream_id,<<"8899be943eaa8885">>}]},
        5000],
       [{file,
          "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,136}]},
     {inbox_helper,'-given_conversations_between/2-fun-1-',4,
       [{file,"/home/circleci/project/big_tests/tests/inbox_helper.erl"},
        {line,543}]},
     {lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
 ...

Report log


riak_mnesia_24 / riak_mnesia / 05322df
Reports root/ big
OK: 2495 / Failed: 0 / User-skipped: 634 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 05322df
Reports root/ big
OK: 2299 / Failed: 0 / User-skipped: 661 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 05322df
Reports root/ big
OK: 2657 / Failed: 0 / User-skipped: 642 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 05322df
Reports root/ big
OK: 4465 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 05322df
Reports root/ big
OK: 4451 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 05322df
Reports root/ big
OK: 4465 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 05322df
Reports root/ big
OK: 4163 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 05322df
Reports root/ big
OK: 4137 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0

Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

All clear, good one 👌🏽

@NelsonVides NelsonVides merged commit 7849967 into feature/mongoose_c2s Jan 23, 2023
@NelsonVides NelsonVides deleted the c2s/enable-remaining-suites branch January 23, 2023 16:46
@jacekwegr jacekwegr added this to the 6.1.0 milestone Apr 27, 2023
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