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

feat: allow modifying c2s session info #3532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pouriya
Copy link
Contributor

@pouriya pouriya commented Feb 17, 2021

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 35.083% when pulling 821bf93 on pouriya:allow-modifying-c2s-session-info into 7da033f on processone:master.

@slezakattack
Copy link
Contributor

What is the advantage of adding this hook as opposed to listening on the "sm_register_connection_hook" and calling "ejabberd_sm:set_user_info"? What's the specific use case you're trying to solve?

@pouriya
Copy link
Contributor Author

pouriya commented Feb 25, 2021

  • ejabberd_sm:set_user_info/5 means getting all sessions (which is a cheap operation) and update one of them which is not cheap since it should be updated on all nodes (in case of using Mnesia). So if we update/write to ejabberd_sm backend once for each c2s user (I know there's other checks for resource conflict, etc), This makes it twice and if we want to put more KV in Info, we have to call it again and again. The new hook sm_pre_register_connection_hook fixes this.
  • How should we use ejabberd_sm:set_user_info/5 to put our custom KV to Info? Sounds like it's better to use sm_register_connection_hook which is called after ejabberd_sm:set_session/6 and ejabberd_sm:check_for_sessions_to_replace/3 (in ejabberd_sm:open_session/6).
    But what happens to checks to saved sessions that don't have our custom Info yet? because ejabberd_sm:check_for_sessions_to_replace/3 is running and our hook callback module waits to be called after this. The new hook sm_pre_register_connection_hook fixes this.

@slezakattack My custom use case is that I can (parse our custom resource and) put user-agent, device unique id, device push notification id, etc in Info right after binding resource and before saving session to ejabberd_sm backend.

@pouriya
Copy link
Contributor Author

pouriya commented Feb 25, 2021

@slezakattack I also wanted to know that are you okay with changing Info type from proplist to map?

Erlang/OTP 23 [erts-11.0] [source] [64-bit] [smp:6:6] [ds:6:6:10] [async-threads:1] [hipe]

Eshell V11.0  (abort with ^G)
1> P = [{ip, {127,0,0,1}}, {conn, c2s}, {auth_module, ejabberd_auth_jwt}].
[{ip,{127,0,0,1}},{conn,c2s},{auth_module,ejabberd_auth_jwt}]

2> M = maps:from_list(P).                                                 
#{auth_module => ejabberd_auth_jwt,conn => c2s,ip => {127,0,0,1}}

% Returns the size of Term in actual heap words. Shared subterms are counted once.
3> erts_debug:size(P).
20

4> erts_debug:size(M).
15

5> RetryCount = lists:seq(1, 100000),ok.
ok

% I lookup 'conn' which is in the middle of list:
6> timer:tc(fun() -> lists:foreach(fun(_) -> proplists:get_value(conn, P) end, RetryCount) end). 
{66721,ok}

7> timer:tc(fun() -> lists:foreach(fun(_) -> maps:get(conn, M) end, RetryCount) end).
{64413,ok}

Also map is better in pattern-matching than proplist.

@weiss
Copy link
Member

weiss commented Feb 25, 2021

My custom use case is that I can (parse our custom resource and) put user-agent, device unique id, device push notification id, etc in Info right after binding resource and before saving session to ejabberd_sm backend.

Adding those things to the ejabberd_c2s state (via c2s_session_opened and c2s_session_resumed) won't do the trick? I.e. it needs to be in the ejabberd_sm backend?

@pouriya
Copy link
Contributor Author

pouriya commented Feb 25, 2021

@weiss ejabberd_c2s keeps other things. For example I collect some ACL stuff from JWT and use them in ejabberd_c2s to control user behavior. ejabberd_sm helps me in two ways, 1st I used sm_receive_packet to implement custom routing of some messages based on session info. I also do not need offline and privacy hooks and checking for online user max priority and I drop some messages in sm_receive_packet callback which increases mps. 2nd I use this data for ReST-API-routing/statistic/monitoring/debugging and it's so fast.

@slezakattack
Copy link
Contributor

slezakattack commented Mar 1, 2021

@slezakattack I also wanted to know that are you okay with changing Info type from proplist to map?

I don't have a strong opinion on which datatype to use but I think migrating to a different datatype will incur migration costs to those who want to upgrade their ejabberd servers. I'm not sure if a 3% speed improvement is worth that cost but I'm not on the ejabberd team, just a member of the community. Just my two cents.

@pouriya
Copy link
Contributor Author

pouriya commented Mar 2, 2021

@slezakattack Good point. It can be backward compatible for some times.

@pouriya
Copy link
Contributor Author

pouriya commented Apr 3, 2021

ping

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.

4 participants