From fd36726357bda81003fa0a76055db13ef6db2288 Mon Sep 17 00:00:00 2001 From: Christophe Romain Date: Mon, 11 May 2009 17:27:55 +0000 Subject: [PATCH] Prevent race condition when calling get_caps while note_caps has not been handled yet (EJAB-934) SVN Revision: 2072 --- ChangeLog | 6 ++ src/mod_caps.erl | 64 +++++++++++++++++----- src/mod_pubsub/mod_pubsub.erl | 100 +++++++++++++++++----------------- 3 files changed, 106 insertions(+), 64 deletions(-) diff --git a/ChangeLog b/ChangeLog index 12ae00993..08bc3c68e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2009-05-11 Christophe Romain + + * src/mod_caps.erl: Prevent race condition when calling get_caps while + note_caps has not been handled yet (EJAB-934) + * src/mod_pubsub/mod_pubsub.erl: Likewise + 2009-05-08 Christophe Romain * src/mod_pubsub/mod_pubsub.erl: Allow to get subscriptions on a given diff --git a/src/mod_caps.erl b/src/mod_caps.erl index 532cc8fa1..1a6691cc0 100644 --- a/src/mod_caps.erl +++ b/src/mod_caps.erl @@ -35,6 +35,7 @@ -export([read_caps/1, get_caps/1, note_caps/3, + wait_caps/2, clear_caps/1, get_features/2, get_user_resources/2, @@ -57,7 +58,8 @@ %% hook handlers -export([receive_packet/3, - receive_packet/4]). + receive_packet/4, + presence_probe/3]). -include("ejabberd.hrl"). @@ -92,12 +94,25 @@ read_caps([], Result) -> Result. %% get_caps reads user caps from database -get_caps({U, S, R}) -> +%% here we handle a simple retry loop, to avoid race condition +%% when asking caps while we still did not called note_caps +%% timeout is set to 10s +%% this is to be improved, but without altering performances. +%% if we did not get user presence 10s after getting presence_probe +%% we assume it has no caps +get_caps(LJID) -> + get_caps(LJID, 5). +get_caps(_, 0) -> + nothing; +get_caps({U, S, R}, Retry) -> BJID = exmpp_jid:jid_to_binary(U, S, R), case catch mnesia:dirty_read({user_caps, BJID}) of - [#user_caps{caps=Caps}] -> + [#user_caps{caps=waiting}] -> + timer:sleep(2000), + get_caps({U, S, R}, Retry-1); + [#user_caps{caps=Caps}] -> Caps; - _ -> + _ -> nothing end. @@ -132,6 +147,13 @@ note_caps(Host, From, Caps) -> gen_server:cast(Proc, {note_caps, From, Caps}) end. +%% wait_caps should be called just before note_caps +%% it allows to lock get_caps usage for code using presence_probe +%% that may run before we get any chance to note_caps. +wait_caps(Host, From) -> + Proc = gen_mod:get_module_proc(Host, ?PROCNAME), + gen_server:cast(Proc, {wait_caps, From}). + %% get_features returns a list of features implied by the given caps %% record (as extracted by read_caps). It may block, and may signal a %% timeout error. @@ -192,6 +214,10 @@ receive_packet(_, _, _) -> receive_packet(_JID, From, To, Packet) -> receive_packet(From, To, Packet). +presence_probe(From, To, _) -> + ServerString = exmpp_jid:ldomain_as_list(To), + wait_caps(ServerString, From). + caps_to_binary(#caps{node = Node, version = Version, exts = Exts}) -> BExts = [list_to_binary(Ext) || Ext <- Exts], #caps{node = list_to_binary(Node), version = list_to_binary(Version), exts = BExts}. @@ -218,10 +244,11 @@ init([Host, _Opts]) -> {type, bag}, {attributes, record_info(fields, user_caps_resources)}]), mnesia:delete_table(user_caps_default), - mnesia:clear_table(user_caps), - mnesia:clear_table(user_caps_resources), + mnesia:clear_table(user_caps), % clean in case of explicitely set to disc_copies + mnesia:clear_table(user_caps_resources), % clean in case of explicitely set to disc_copies ejabberd_hooks:add(user_receive_packet, Host, ?MODULE, receive_packet, 30), ejabberd_hooks:add(s2s_receive_packet, Host, ?MODULE, receive_packet, 30), + ejabberd_hooks:add(presence_probe_hook, Host, ?MODULE, presence_probe, 20), {ok, #state{host = Host}}. maybe_get_features(#caps{node = Node, version = Version, exts = Exts}) -> @@ -273,15 +300,17 @@ handle_cast({note_caps, From, S = exmpp_jid:ldomain(From), R = exmpp_jid:resource(From), BJID = exmpp_jid:jid_to_binary(From), - mnesia:dirty_write(#user_caps{jid = BJID, caps = caps_to_binary(Caps)}), - case ejabberd_sm:get_user_resources(U, S) of - [] -> - % only store resource of caps aware external contacts - BUID = exmpp_jid:bare_jid_to_binary(From), - mnesia:dirty_write(#user_caps_resources{uid = BUID, resource = list_to_binary(R)}); - _ -> - ok - end, + mnesia:transaction(fun() -> + mnesia:dirty_write(#user_caps{jid = BJID, caps = caps_to_binary(Caps)}), + case ejabberd_sm:get_user_resources(U, S) of + [] -> + % only store resource of caps aware external contacts + BUID = exmpp_jid:bare_jid_to_binary(From), + mnesia:dirty_write(#user_caps_resources{uid = BUID, resource = list_to_binary(R)}); + _ -> + ok + end + end), %% Now, find which of these are not already in the database. SubNodes = [Version | Exts], case lists:foldl(fun(SubNode, Acc) -> @@ -312,6 +341,10 @@ handle_cast({note_caps, From, end, Requests, Missing), {noreply, State#state{disco_requests = NewRequests}} end; +handle_cast({wait_caps, From}, State) -> + BJID = exmpp_jid:jid_to_binary(From), + mnesia:dirty_write(#user_caps{jid = BJID, caps = waiting}), + {noreply, State}; handle_cast({disco_response, From, _To, #iq{id = ID, type = Type, payload = Payload}}, #state{disco_requests = Requests} = State) -> case {Type, Payload} of @@ -384,6 +417,7 @@ terminate(_Reason, State) -> Host = State#state.host, ejabberd_hooks:delete(user_receive_packet, Host, ?MODULE, receive_packet, 30), ejabberd_hooks:delete(s2s_receive_packet, Host, ?MODULE, receive_packet, 30), + ejabberd_hooks:delete(presence_probe_hook, Host, ?MODULE, presence_probe, 20), ok. code_change(_OldVsn, State, _Extra) -> diff --git a/src/mod_pubsub/mod_pubsub.erl b/src/mod_pubsub/mod_pubsub.erl index 543768239..d5259bf2e 100644 --- a/src/mod_pubsub/mod_pubsub.erl +++ b/src/mod_pubsub/mod_pubsub.erl @@ -179,7 +179,7 @@ init([ServerHost, Opts]) -> ejabberd_hooks:add(disco_sm_identity, ServerHostB, ?MODULE, disco_sm_identity, 75), ejabberd_hooks:add(disco_sm_features, ServerHostB, ?MODULE, disco_sm_features, 75), ejabberd_hooks:add(disco_sm_items, ServerHostB, ?MODULE, disco_sm_items, 75), - ejabberd_hooks:add(presence_probe_hook, ServerHostB, ?MODULE, presence_probe, 50), + ejabberd_hooks:add(presence_probe_hook, ServerHostB, ?MODULE, presence_probe, 80), ejabberd_hooks:add(roster_in_subscription, ServerHostB, ?MODULE, in_subscription, 50), ejabberd_hooks:add(roster_out_subscription, ServerHostB, ?MODULE, out_subscription, 50), ejabberd_hooks:add(remove_user, ServerHostB, ?MODULE, remove_user, 50), @@ -448,57 +448,59 @@ send_loop(State) -> end, send_loop(State); {presence, User, Server, Resources, JID} -> -?INFO_MSG("got presence probe ~s@~s~p ~p",[User,Server,Resources,jlib:jid_tolower(JID)]), %% get resources caps and check if processing is needed - {HasCaps, ResourcesCaps} = lists:foldl(fun(Resource, {R, L}) -> - case mod_caps:get_caps({User, Server, Resource}) of - nothing -> {R, L}; - Caps -> {true, [{Resource, Caps} | L]} - end - end, {false, []}, Resources), - case HasCaps of - true -> - Host = State#state.host, - ServerHost = State#state.server_host, - Owner = jlib:short_prepd_bare_jid(JID), - lists:foreach(fun(#pubsub_node{nodeid = {_, Node}, type = Type, id = NodeId, options = Options}) -> - case get_option(Options, send_last_published_item) of - on_sub_and_presence -> - lists:foreach(fun({Resource, Caps}) -> - CapsNotify = case catch mod_caps:get_features(ServerHost, Caps) of - Features when is_list(Features) -> lists:member(Node ++ "+notify", Features); - _ -> false - end, -?INFO_MSG("notify ~s ~s",[Node, CapsNotify]), - case CapsNotify of - true -> - LJID = {User, Server, Resource}, - Subscribed = case get_option(Options, access_model) of - open -> true; - presence -> true; - whitelist -> false; % subscribers are added manually - authorize -> false; % likewise - roster -> - Grps = get_option(Options, roster_groups_allowed, []), - {OU, OS, _} = Owner, - element(2, get_roster_info(OU, OS, LJID, Grps)) + %% get_caps may be blocked few seconds, get_caps as well + %% so we spawn the whole process not to block other queries + spawn(fun() -> + {HasCaps, ResourcesCaps} = lists:foldl(fun(Resource, {R, L}) -> + case mod_caps:get_caps({User, Server, Resource}) of + nothing -> {R, L}; + Caps -> {true, [{Resource, Caps} | L]} + end + end, {false, []}, Resources), + case HasCaps of + true -> + Host = State#state.host, + ServerHost = State#state.server_host, + Owner = jlib:short_prepd_bare_jid(JID), + lists:foreach(fun(#pubsub_node{nodeid = {_, Node}, type = Type, id = NodeId, options = Options}) -> + case get_option(Options, send_last_published_item) of + on_sub_and_presence -> + lists:foreach(fun({Resource, Caps}) -> + CapsNotify = case catch mod_caps:get_features(ServerHost, Caps) of + Features when is_list(Features) -> lists:member(Node ++ "+notify", Features); + _ -> false end, - if Subscribed -> - send_items(Owner, Node, NodeId, Type, LJID, last); + case CapsNotify of true -> + LJID = {User, Server, Resource}, + Subscribed = case get_option(Options, access_model) of + open -> true; + presence -> true; + whitelist -> false; % subscribers are added manually + authorize -> false; % likewise + roster -> + Grps = get_option(Options, roster_groups_allowed, []), + {OU, OS, _} = Owner, + element(2, get_roster_info(OU, OS, LJID, Grps)) + end, + if Subscribed -> + send_items(Owner, Node, NodeId, Type, LJID, last); + true -> + ok + end; + false -> ok - end; - false -> - ok - end - end, ResourcesCaps); - _ -> - ok - end - end, tree_action(Host, get_nodes, [Owner, JID])); - false -> - ok - end, + end + end, ResourcesCaps); + _ -> + ok + end + end, tree_action(Host, get_nodes, [Owner, JID])); + false -> + ok + end + end), send_loop(State); stop -> ok @@ -800,7 +802,7 @@ terminate(_Reason, #state{host = Host, ejabberd_hooks:delete(disco_sm_identity, ServerHostB, ?MODULE, disco_sm_identity, 75), ejabberd_hooks:delete(disco_sm_features, ServerHostB, ?MODULE, disco_sm_features, 75), ejabberd_hooks:delete(disco_sm_items, ServerHostB, ?MODULE, disco_sm_items, 75), - ejabberd_hooks:delete(presence_probe_hook, ServerHostB, ?MODULE, presence_probe, 50), + ejabberd_hooks:delete(presence_probe_hook, ServerHostB, ?MODULE, presence_probe, 80), ejabberd_hooks:delete(roster_in_subscription, ServerHostB, ?MODULE, in_subscription, 50), ejabberd_hooks:delete(roster_out_subscription, ServerHostB, ?MODULE, out_subscription, 50), ejabberd_hooks:delete(remove_user, ServerHostB, ?MODULE, remove_user, 50),