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

Accept error already_started and make sure nksip app is running #1960

Merged
merged 1 commit into from
Jul 5, 2018

Conversation

michalwski
Copy link
Contributor

This PR makes sure that nksip app is started while starting mod_jingle_sip. Also it allows to start the module for more than just one host.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Jul 5, 2018

5049.1 / Erlang 19.3 / small_tests / 12435f8
Reports root / small


5049.5 / Erlang 19.3 / ldap_mnesia / 12435f8
Reports root/ big
OK: 1024 / Failed: 0 / User-skipped: 77 / Auto-skipped: 0


5049.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / 12435f8
Reports root/ big
OK: 445 / Failed: 0 / User-skipped: 8 / Auto-skipped: 0


5049.2 / Erlang 19.3 / internal_mnesia / 12435f8
Reports root/ big
OK: 1060 / Failed: 0 / User-skipped: 41 / Auto-skipped: 0


5049.3 / Erlang 19.3 / mysql_redis / 12435f8
Reports root/ big
OK: 2799 / Failed: 0 / User-skipped: 222 / Auto-skipped: 0


5049.4 / Erlang 19.3 / odbc_mssql_mnesia / 12435f8
Reports root/ big
OK: 2794 / Failed: 0 / User-skipped: 209 / Auto-skipped: 0


5049.8 / Erlang 20.0 / pgsql_mnesia / 12435f8
Reports root/ big / small
OK: 2845 / Failed: 0 / User-skipped: 176 / Auto-skipped: 0


5049.9 / Erlang 21.0 / riak_mnesia / 12435f8
Reports root/ big / small
OK: 1279 / Failed: 0 / User-skipped: 43 / Auto-skipped: 0

@@ -71,7 +82,6 @@ maybe_add_udp_max_size(NkSipOpts, Opts) ->
-spec stop(ejabberd:server()) -> ok.
stop(Host) ->
ejabberd_hooks:delete(hooks(Host)),
nksip:stop(?SERVICE),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe part of Jingle/SIP extension could be extracted into service_...? Not stopping nksip seems a bit like a temporary solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I removed the stop here since there may be many xmpp hosts served by MongooseIM. This service is shared between them so stopping it with first host stopped is bad idea, it's better to let it operate on stop manually if needed.
  2. I tried service_ framework, I was not able to use it without modifications. Below is what I miss or makes things more difficult:
    1. currently if I want to have a service started for a module, it needs to be explicitly added in the config file in the services section. This complicates modules configuration since the config is split into 2 places - the service and the module. I think it be more useful to allow configuring the service via module's options.
    2. even I start nksip as a service, there is still no mechanism which will stop it when all modules using it are stopped.

Both items from point 2 are not very small changes, that's why I decided not to go with the service framework. I think nksip service is perfect fit for the service framework when it supports what's missing for me at this moment.

Copy link
Member

Choose a reason for hiding this comment

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

OK, understood, I agree it's not a big deal to let it running. Maybe some yet another alternative solution will emerge over time.

@codecov
Copy link

codecov bot commented Jul 5, 2018

Codecov Report

Merging #1960 into master will increase coverage by 0.07%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1960      +/-   ##
=========================================
+ Coverage   74.83%   74.9%   +0.07%     
=========================================
  Files         312     312              
  Lines       28421   28425       +4     
=========================================
+ Hits        21269   21292      +23     
+ Misses       7152    7133      -19
Impacted Files Coverage Δ
src/jingle_sip/mod_jingle_sip.erl 80.54% <77.77%> (-0.49%) ⬇️
...rc/global_distrib/mod_global_distrib_transport.erl 47.05% <0%> (-5.89%) ⬇️
src/mam/mod_mam_muc_odbc_async_pool_writer.erl 62.76% <0%> (-4.26%) ⬇️
src/mod_muc_log.erl 77.69% <0%> (ø) ⬆️
src/pubsub/mod_pubsub.erl 65.99% <0%> (+0.06%) ⬆️
src/ejabberd_sm.erl 84.17% <0%> (+0.67%) ⬆️
...c/global_distrib/mod_global_distrib_server_mgr.erl 80.28% <0%> (+0.7%) ⬆️
src/muc_light/mod_muc_light_utils.erl 85.26% <0%> (+1.05%) ⬆️
src/mod_bosh.erl 94.95% <0%> (+1.68%) ⬆️
src/pubsub/gen_pubsub_node.erl 12.5% <0%> (+3.12%) ⬆️
... and 1 more

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 47b2c79...07e1aa8. Read the comment docs.

@fenek
Copy link
Member

fenek commented Jul 5, 2018

According to user's report in #1946 the startup problem is fixed, so I'm merging it. Any further issues will be handled in separate PR.

@fenek fenek merged commit ad2c77e into master Jul 5, 2018
@fenek fenek deleted the fix-mod-jing-sip-start branch July 5, 2018 10:49
@fenek fenek added this to the 3.0.0++ milestone Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants