feat: allow modifying c2s session info

[pouriya]

No description provided.

[coveralls]

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

[slezakattack]

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]

• 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]

@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]

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]

@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]

@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]

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

[pouriya]

ping