C2s/ejabberd #3805

kamilwaz · 2022-10-11T07:56:02Z

Remove ejabberd_c2s module completely.

codecov · 2022-10-11T14:51:49Z

Codecov Report

Base: 70.72% // Head: 71.95% // Increases project coverage by +1.23% 🎉

Coverage data is based on head (c767a27) compared to base (bbc6885).
Patch coverage: 73.83% of modified lines in pull request are covered.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/mongoose_c2s    #3805      +/-   ##
========================================================
+ Coverage                 70.72%   71.95%   +1.23%     
========================================================
  Files                       536      535       -1     
  Lines                     35209    33922    -1287     
========================================================
- Hits                      24900    24408     -492     
+ Misses                    10309     9514     -795

Impacted Files	Coverage Δ
src/ejabberd_router.erl	`78.57% <ø> (ø)`
src/ejabberd_socket.erl	`31.16% <ø> (+1.90%)`	⬆️
src/ejabberd_sup.erl	`85.71% <ø> (ø)`
src/ejabberd_zlib.erl	`0.00% <ø> (ø)`
src/jlib.erl	`80.34% <ø> (-1.16%)`	⬇️
src/mod_ping.erl	`98.43% <ø> (ø)`
...rc/mongoose_client_api/mongoose_client_api_sse.erl	`58.97% <ø> (ø)`
src/mongoose_hooks.erl	`89.40% <0.00%> (+2.61%)`	⬆️
src/mongoose_iq.erl	`85.00% <ø> (ø)`
src/mongoose_tls.erl	`2.50% <ø> (ø)`
... and 45 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 · 2022-10-26T11:48:51Z

small_tests_24 / small_tests / ed9588b
Reports root / small

small_tests_25 / small_tests / ed9588b
Reports root / small

ldap_mnesia_24 / ldap_mnesia / ed9588b
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / ed9588b
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / ed9588b
Reports root/ big
OK: 3544 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / ed9588b
Reports root/ big
OK: 1634 / Failed: 0 / User-skipped: 597 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / ed9588b
Reports root/ big
OK: 3720 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / ed9588b
Reports root/ big
OK: 1968 / Failed: 0 / User-skipped: 598 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / ed9588b
Reports root/ big
OK: 3544 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / ed9588b
Reports root/ big
OK: 3518 / Failed: 0 / User-skipped: 104 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / ed9588b
Reports root/ big
OK: 1808 / Failed: 0 / User-skipped: 590 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / ed9588b
Reports root/ big
OK: 3720 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / ed9588b
Reports root/ big
OK: 3706 / Failed: 0 / User-skipped: 103 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / ed9588b
Reports root/ big
OK: 3716 / Failed: 4 / User-skipped: 89 / Auto-skipped: 0

service_domain_db_SUITE:db:db_could_sync_between_nodes

{error,
  {{badrpc,{'EXIT',{timeout,{gen_server,call,[service_domain_db,ping]}}}},
   [{distributed_helper,rpc,
      [#{node => mongooseim@localhost},service_domain_db,sync_local,[]],
      [{file,
         "/home/circleci/project/big_tests/tests/distributed_helper.erl"},
       {line,117}]},
    {service_domain_db_SUITE,sync_local,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1175}]},
    {service_domain_db_SUITE,sync,0,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1154}]},
    {service_domain_db_SUITE,db_could_sync_between_nodes,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,556}]},
    {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

service_domain_db_SUITE:db:db_deleted_from_one_node_while_service_disabled_on_another

{error,
  {{badrpc,timeout},
   [{distributed_helper,rpc,
      [#{node => mongooseim@localhost},service_domain_db,sync_local,[]],
      [{file,
         "/home/circleci/project/big_tests/tests/distributed_helper.erl"},
       {line,117}]},
    {service_domain_db_SUITE,sync_local,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1175}]},
    {service_domain_db_SUITE,sync,0,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1154}]},
    {service_domain_db_SUITE,
      db_deleted_from_one_node_while_service_disabled_on_another,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,561}]},
    {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

service_domain_db_SUITE:db:db_inserted_from_one_node_while_service_disabled_on_another

{error,
  {{badrpc,timeout},
   [{distributed_helper,rpc,
      [#{node => mongooseim2@localhost},service_domain_db,sync_local,[]],
      [{file,
         "/home/circleci/project/big_tests/tests/distributed_helper.erl"},
       {line,117}]},
    {service_domain_db_SUITE,sync_local,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1175}]},
    {service_domain_db_SUITE,
      db_inserted_from_one_node_while_service_disabled_on_another,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,579}]},
    {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

service_domain_db_SUITE:db:db_reinserted_from_one_node_while_service_disabled_on_another

{error,
  {{badrpc,timeout},
   [{distributed_helper,rpc,
      [#{node => mongooseim@localhost},service_domain_db,sync_local,[]],
      [{file,
         "/home/circleci/project/big_tests/tests/distributed_helper.erl"},
       {line,117}]},
    {service_domain_db_SUITE,sync_local,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1175}]},
    {service_domain_db_SUITE,sync,0,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,1154}]},
    {service_domain_db_SUITE,
      db_reinserted_from_one_node_while_service_disabled_on_another,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,586}]},
    {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_mssql_mnesia_25 / odbc_mssql_mnesia / ed9588b
Reports root/ big
OK: 3544 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / ed9588b
Reports root/ big
OK: 3720 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

NelsonVides · 2022-10-26T13:31:02Z

rebar.config

@@ -112,8 +112,7 @@

  %%% Other
  {pa, {git, "https://github.com/erszcz/pa.git", {branch, "master"}}},
-  %% MR of jwerl - https://gitlab.com/glejeune/jwerl/-/merge_requests/13
-  {jwerl, {git, "https://gitlab.com/vkatsuba/jwerl.git", {branch, "refactoring/otp-24"}}},
+  {jwerl, {git, "https://gitlab.com/glejeune/jwerl.git", {tag, "1.2.0"}}},


Here we could use the hex version already btw.

NelsonVides

A few comments made, otherwise what an outstanding work! 😃

NelsonVides · 2022-10-26T13:39:14Z

src/mod_caps.erl

-            Rs = case ejabberd_c2s:get_aux_field(caps_resources,
-                                                 C2SState)
-                 of
+            Rs = case mongoose_c2s:get_mod_state(StateData, ?MODULE) of


I'm sad dialyzer doesn't fail on this one. get_mod_state will return {error, not_found} | term(), so this pattern-matching will very likely fail. Please re-check that the state stored is what is expected here, the ok tuple also seems off from a distance 🤔

NelsonVides · 2022-10-26T13:39:56Z

src/mod_caps.erl

         c2s_broadcast_recipients/5]).

 %% for test cases
 -export([delete_caps/1, make_disco_hash/2]).

-ignore_xref([c2s_broadcast_recipients/5, c2s_filter_packet/5, c2s_presence_in/4,
+-ignore_xref([c2s_broadcast_recipients/5, c2s_filter_packet/5, user_receive_presence/3,


This new hooks don't need to be added to ignore_xref, as we refer to them by their function object (fun ?MODULE:Function/3), xref knows they're used.

NelsonVides · 2022-10-26T14:09:59Z

src/c2s/mongoose_c2s_ranch.erl

+-spec has_peer_cert(mongoose_c2s_socket:state(), mongoose_listener:options()) -> boolean().
+has_peer_cert(#state{transport = fast_tls, socket = Socket}, #{tls := TlsOpts}) ->
+    case {fast_tls:get_verify_result(Socket), fast_tls:get_peer_certificate(Socket), TlsOpts} of
+        {0, {ok, _}, _} -> true;
+        {18, {ok, _}, #{verify_mode := selfsigned_peer}} -> true;
+        {_, {ok, _}, _} -> false;
+        {_, error, _} -> false


Here you could move the comment from mongoose_c2s_socket that says %% 18 is OpenSSL's and fast_tls's error code for self-signed certs, so that that 18 ain't so magic.

NelsonVides · 2022-10-26T14:23:15Z

src/ejabberd_sm.erl

@@ -928,7 +907,7 @@ check_for_sessions_to_replace(HostType, JID) ->
    %% replacement for max_sessions. We need to check this at some point.
    ReplacedRedundantSessions = check_existing_resources(HostType, LUser, LServer, LResource),
    AllReplacedSessionPids = check_max_sessions(HostType, LUser, LServer, ReplacedRedundantSessions),
-    [Pid ! replaced || Pid <- AllReplacedSessionPids],
+    [mongoose_c2s:exit(Pid, <<"Replaced by new connection">>) || Pid <- AllReplacedSessionPids],


After this, we could remove the handle_info clause in mongoose_c2s that was catching the replaced message.

NelsonVides · 2022-10-26T14:24:35Z

src/mod_bosh_socket.erl

+                hibernate_after => 0,
+                c2s_state_timeout => 5000,
+                backwards_compatible_session => true},


Huh, we should have these from the config, but anyway, nobody uses bosh, nor they should anyway 🤷🏽

NelsonVides · 2022-10-26T14:30:06Z

src/mod_websockets.erl

+        hibernate_after => 0,
+        c2s_state_timeout => 5000,
+        backwards_compatible_session => true},


Here's a bit more important to have the correct config, websockets are ubiquitous. You can check them on the listener, using some mongoose_config function to get the listener config and then extract them. Or maybe better, they should be kept in the websocket state, #ws_state{}, from there we can correctly configure this.

NelsonVides

Couple more comments I just noticed

NelsonVides · 2022-10-26T19:53:22Z

src/c2s/mongoose_c2s_socket.erl

+            {ok, C2SSocket#c2s_socket{state = NewState}};
+        Error ->
+            Error
+        end.


Indentation is off

NelsonVides · 2022-10-26T19:54:36Z

src/c2s/mongoose_c2s_socket.erl

+-spec handle_data(socket(), {tcp | ssl, term(), iodata()}) ->
+    iodata() | {raw, [term()]} | {error, term()}.
+handle_data(#c2s_socket{module = Module, state = State},
+            {Transport, _Socket, _Data} = Payload) when Transport == tcp; Transport == ssl ->


This is only called by mongoose_c2s after it has already verified that the Transport tag is one of tcp or ssl, so it is probably not necessary here and we can expect dialyzer to catch any other related issue.

mongoose-im · 2022-10-27T09:12:26Z

small_tests_24 / small_tests / c767a27
Reports root / small

small_tests_25 / small_tests / c767a27
Reports root / small

ldap_mnesia_24 / ldap_mnesia / c767a27
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / c767a27
Reports root/ big
OK: 1545 / Failed: 0 / User-skipped: 686 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c767a27
Reports root/ big
OK: 3544 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / c767a27
Reports root/ big
OK: 3720 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / c767a27
Reports root/ big
OK: 1634 / Failed: 0 / User-skipped: 597 / Auto-skipped: 0

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / c767a27
Reports root/ big
OK: 1968 / Failed: 0 / User-skipped: 598 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / c767a27
Reports root/ big
OK: 3544 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / c767a27
Reports root/ big
OK: 3518 / Failed: 0 / User-skipped: 104 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / c767a27
Reports root/ big
OK: 1808 / Failed: 0 / User-skipped: 590 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / c767a27
Reports root/ big
OK: 3706 / Failed: 0 / User-skipped: 103 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / c767a27
Reports root/ big
OK: 3720 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / c767a27
Reports root/ big
OK: 3719 / Failed: 1 / User-skipped: 89 / Auto-skipped: 0

service_domain_db_SUITE:db:db_event_could_appear_with_lower_id

{error,
  {{badmatch,467},
   [{service_domain_db_SUITE,db_event_could_appear_with_lower_id,1,
      [{file,
         "/home/circleci/project/big_tests/tests/service_domain_db_SUITE.erl"},
       {line,686}]},
    {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_mssql_mnesia_25 / odbc_mssql_mnesia / c767a27
Reports root/ big
OK: 3544 / Failed: 0 / User-skipped: 78 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / c767a27
Reports root/ big
OK: 3720 / Failed: 0 / User-skipped: 89 / Auto-skipped: 0

NelsonVides

I have no further comments, all left has been well captured in the backlog. Thanks so much for this big piece of work! 🎉🎉🎉

kamilwaz changed the base branch from master to feature/mongoose_c2s October 11, 2022 07:56

kamilwaz force-pushed the c2s/ejabberd branch from 5865f40 to 38c2219 Compare October 11, 2022 07:59

This comment was marked as outdated.

Sign in to view

mongoose-im mentioned this pull request Oct 11, 2022

CI build failures summary - part 2 #3685

Closed

This comment was marked as outdated.

Sign in to view

kamilwaz added the mongoose_c2s label Oct 12, 2022

kamilwaz force-pushed the c2s/ejabberd branch from 3668310 to af3a4cc Compare October 19, 2022 13:34

This comment was marked as outdated.

Sign in to view

kamilwaz force-pushed the c2s/ejabberd branch from af3a4cc to c8155b6 Compare October 21, 2022 06:59

This comment was marked as outdated.

Sign in to view

Remove ejabberd_c2s

6102957

kamilwaz force-pushed the c2s/ejabberd branch from 86f7795 to 6102957 Compare October 25, 2022 08:51

This comment was marked as outdated.

Sign in to view

Kamil Waz added 2 commits October 26, 2022 13:40

Update route and replaced messages

ecee18d

Remove ejabberd_c2s from mod_caps

ed9588b

kamilwaz force-pushed the c2s/ejabberd branch from 3f48827 to ed9588b Compare October 26, 2022 11:40

kamilwaz marked this pull request as ready for review October 26, 2022 12:31

kamilwaz requested a review from NelsonVides October 26, 2022 12:31

NelsonVides reviewed Oct 26, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

Apply comments from review

c767a27

kamilwaz force-pushed the c2s/ejabberd branch from 651690d to c767a27 Compare October 27, 2022 09:03

kamilwaz requested a review from NelsonVides October 27, 2022 09:55

NelsonVides approved these changes Oct 28, 2022

View reviewed changes

NelsonVides merged commit 49eb66c into feature/mongoose_c2s Oct 28, 2022

NelsonVides deleted the c2s/ejabberd branch October 28, 2022 02:48

jacekwegr added this to the 6.1.0 milestone Apr 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C2s/ejabberd #3805

C2s/ejabberd #3805

kamilwaz commented Oct 11, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

codecov bot commented Oct 11, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mongoose-im commented Oct 26, 2022 •

edited

Loading

NelsonVides Oct 26, 2022

NelsonVides left a comment

NelsonVides Oct 26, 2022

NelsonVides Oct 26, 2022

NelsonVides Oct 26, 2022

NelsonVides Oct 26, 2022

NelsonVides Oct 26, 2022

NelsonVides Oct 26, 2022

NelsonVides left a comment

NelsonVides Oct 26, 2022

NelsonVides Oct 26, 2022

This comment was marked as outdated.

mongoose-im commented Oct 27, 2022 •

edited

Loading

NelsonVides left a comment

C2s/ejabberd #3805

C2s/ejabberd #3805

Conversation

kamilwaz commented Oct 11, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

codecov bot commented Oct 11, 2022 • edited Loading

Codecov Report

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mongoose-im commented Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

NelsonVides left a 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

Choose a reason for hiding this comment

NelsonVides left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

mongoose-im commented Oct 27, 2022 • edited Loading

NelsonVides left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2022 •

edited

Loading

mongoose-im commented Oct 26, 2022 •

edited

Loading

mongoose-im commented Oct 27, 2022 •

edited

Loading