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/Do not route broadcast tuples #3946

Merged
merged 7 commits into from
Jan 27, 2023
Merged

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jan 26, 2023

The special {broadcast, _} tuples were routed in ejabberd_sm, as if they were XMPP stanzas. This introduced confusion and made the code awkward, while not providing clear benefits. Some arguments, like From, were made up to fit the route function signature. Additionally, the sm_broadcast hook was misused by mongoose_metrics - it triggered update of a metric that was presence-specific.

Additionally, to avoid more and more expressions like C2SPid ! SomeTuple and gen_statem:call(C2sPid, SomeTuple), API functions for call and cast are added to mongoose_c2s.

The API function have EventTag :: atom() as the second argument. The tag is then passed to the foreign_event handler. This is to encourage and unify consistent naming of foreign events. The tag is a not mandatory hook arg, because some other events (e.g. timeouts) have their own naming (e.g. {timeout, ?MODULE}).

Routing should be done only for stanzas.
Arbitrary messages shouldn't be sent this way - they are already sent
directly in multiple places.
@chrzaszcz chrzaszcz changed the base branch from master to feature/mongoose_c2s January 26, 2023 11:37
@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Base: 83.52% // Head: 83.51% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (6cad140) compared to base (ce82e62).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/mongoose_c2s    #3946      +/-   ##
========================================================
- Coverage                 83.52%   83.51%   -0.01%     
========================================================
  Files                       538      538              
  Lines                     33965    33944      -21     
========================================================
- Hits                      28368    28348      -20     
+ Misses                     5597     5596       -1     
Impacted Files Coverage Δ
src/c2s/mongoose_c2s_hooks.erl 95.45% <ø> (ø)
src/ejabberd_sm.erl 88.92% <ø> (+1.59%) ⬆️
src/metrics/mongoose_metrics_hooks.erl 97.43% <ø> (ø)
src/admin_extra/service_admin_extra_roster.erl 87.91% <100.00%> (+0.50%) ⬆️
src/c2s/mongoose_c2s.erl 87.33% <100.00%> (+0.08%) ⬆️
src/mod_presence.erl 86.06% <100.00%> (ø)
src/mod_roster.erl 78.80% <100.00%> (+0.15%) ⬆️
src/mongoose_hooks.erl 95.84% <100.00%> (-0.02%) ⬇️
src/privacy/mod_blocking.erl 89.38% <100.00%> (+0.19%) ⬆️
src/privacy/mod_privacy.erl 89.12% <100.00%> (+0.03%) ⬆️
... 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.

The events are tagged to encourage meaningful and consistent
categories, and prevent evnts like '{item, _, _}' which are hard to debug.
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz changed the title C2s/no broadcast C2s/Do not route broadcast tuples Jan 26, 2023
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 26, 2023

small_tests_24 / small_tests / 6cad140
Reports root / small


small_tests_25 / small_tests / 6cad140
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 6cad140
Reports root/ big
OK: 4165 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 6cad140
Reports root/ big
OK: 2217 / Failed: 0 / User-skipped: 823 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 6cad140
Reports root/ big
OK: 4165 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 6cad140
Reports root/ big
OK: 4139 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 6cad140
Reports root/ big
OK: 4545 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 6cad140
Reports root/ big
OK: 2217 / Failed: 0 / User-skipped: 823 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 6cad140
Reports root/ big
OK: 2359 / Failed: 0 / User-skipped: 681 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 6cad140
Reports root/ big
OK: 4162 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 6cad140
Reports root/ big
OK: 2555 / Failed: 0 / User-skipped: 654 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 6cad140
Reports root/ big
OK: 2717 / Failed: 0 / User-skipped: 662 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 6cad140
Reports root/ big
OK: 4545 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 6cad140
Reports root/ big
OK: 4531 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 6cad140
Reports root/ big
OK: 4542 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review January 27, 2023 07:41
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.

Absolutely brilliant, love the broadcast being removed, and very good move having the call/cast built into an API with tagged events, better for reading and debugging!
Great job! 🎉

@NelsonVides NelsonVides merged commit a79e63f into feature/mongoose_c2s Jan 27, 2023
@NelsonVides NelsonVides deleted the c2s/no-broadcast branch January 27, 2023 11:27
@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