From 20088d8a9274e07a153c35514c42ba9f5129ad83 Mon Sep 17 00:00:00 2001 From: Christophe Romain Date: Mon, 11 May 2009 17:16:25 +0000 Subject: [PATCH] Prevent race condition when calling get_caps while note_caps has not been handled yet (EJAB-934) SVN Revision: 2071 --- ChangeLog | 6 +++ src/mod_caps.erl | 65 +++++++++++++++++------ src/mod_pubsub/mod_pubsub.erl | 98 ++++++++++++++++++----------------- 3 files changed, 106 insertions(+), 63 deletions(-) diff --git a/ChangeLog b/ChangeLog index dd12c9a25..cb67ce5a2 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 4a17515be..a0defe4b9 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, @@ -55,7 +56,8 @@ %% hook handlers -export([receive_packet/3, - receive_packet/4]). + receive_packet/4, + presence_probe/3]). -include("ejabberd.hrl"). -include("jlib.hrl"). @@ -101,11 +103,24 @@ read_caps([], Result) -> Result. %% get_caps reads user caps from database -get_caps(JID) -> - case catch mnesia:dirty_read({user_caps, jid_to_binary(JID)}) of - [#user_caps{caps=Caps}] -> +%% 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(LJID, Retry) -> + case catch mnesia:dirty_read({user_caps, jid_to_binary(LJID)}) of + [#user_caps{caps=waiting}] -> + timer:sleep(2000), + get_caps(LJID, Retry-1); + [#user_caps{caps=Caps}] -> Caps; - _ -> + _ -> nothing end. @@ -139,6 +154,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. @@ -197,6 +219,9 @@ receive_packet(_, _, _) -> receive_packet(_JID, From, To, Packet) -> receive_packet(From, To, Packet). +presence_probe(From, To, _) -> + wait_caps(To#jid.lserver, From). + jid_to_binary(JID) -> list_to_binary(jlib:jid_to_string(JID)). @@ -226,10 +251,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}) -> @@ -278,15 +304,17 @@ handle_cast({note_caps, From, %% lots of caps disco requests. {U, S, R} = jlib:jid_tolower(From), BJID = 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 resources of caps aware external contacts - BUID = jid_to_binary({U, S, []}), - mnesia:dirty_write(#user_caps_resources{uid = BUID, resource = list_to_binary(R)}); - _ -> - ok - end, + mnesia:transaction(fun() -> + mnesia:write(#user_caps{jid = BJID, caps = caps_to_binary(Caps)}), + case ejabberd_sm:get_user_resources(U, S) of + [] -> + % only store resources of caps aware external contacts + BUID = jid_to_binary({U, S, []}), + mnesia: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) -> @@ -320,6 +348,10 @@ handle_cast({note_caps, From, end, Requests, Missing), {noreply, State#state{disco_requests = NewRequests}} end; +handle_cast({wait_caps, From}, State) -> + BJID = jid_to_binary(From), + mnesia:dirty_write(#user_caps{jid = BJID, caps = waiting}), + {noreply, State}; handle_cast({disco_response, From, _To, #iq{type = Type, id = ID, sub_el = SubEls}}, @@ -394,6 +426,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 9dbd0fead..fa021cc77 100644 --- a/src/mod_pubsub/mod_pubsub.erl +++ b/src/mod_pubsub/mod_pubsub.erl @@ -177,7 +177,7 @@ init([ServerHost, Opts]) -> ejabberd_hooks:add(disco_sm_identity, ServerHost, ?MODULE, disco_sm_identity, 75), ejabberd_hooks:add(disco_sm_features, ServerHost, ?MODULE, disco_sm_features, 75), ejabberd_hooks:add(disco_sm_items, ServerHost, ?MODULE, disco_sm_items, 75), - ejabberd_hooks:add(presence_probe_hook, ServerHost, ?MODULE, presence_probe, 50), + ejabberd_hooks:add(presence_probe_hook, ServerHost, ?MODULE, presence_probe, 80), ejabberd_hooks:add(roster_in_subscription, ServerHost, ?MODULE, in_subscription, 50), ejabberd_hooks:add(roster_out_subscription, ServerHost, ?MODULE, out_subscription, 50), ejabberd_hooks:add(remove_user, ServerHost, ?MODULE, remove_user, 50), @@ -447,54 +447,58 @@ send_loop(State) -> send_loop(State); {presence, User, Server, Resources, 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:jid_remove_resource(jlib:jid_tolower(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, - 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:jid_remove_resource(jlib:jid_tolower(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 @@ -786,7 +790,7 @@ terminate(_Reason, #state{host = Host, ejabberd_hooks:delete(disco_sm_identity, ServerHost, ?MODULE, disco_sm_identity, 75), ejabberd_hooks:delete(disco_sm_features, ServerHost, ?MODULE, disco_sm_features, 75), ejabberd_hooks:delete(disco_sm_items, ServerHost, ?MODULE, disco_sm_items, 75), - ejabberd_hooks:delete(presence_probe_hook, ServerHost, ?MODULE, presence_probe, 50), + ejabberd_hooks:delete(presence_probe_hook, ServerHost, ?MODULE, presence_probe, 80), ejabberd_hooks:delete(roster_in_subscription, ServerHost, ?MODULE, in_subscription, 50), ejabberd_hooks:delete(roster_out_subscription, ServerHost, ?MODULE, out_subscription, 50), ejabberd_hooks:delete(remove_user, ServerHost, ?MODULE, remove_user, 50),