Prevent race condition when calling get_caps while note_caps has not been handled yet (EJAB-934)

SVN Revision: 2072

ChangeLog

@@ -1,3 +1,9 @@
+2009-05-11  Christophe Romain <christophe.romain@process-one.net>
+
+	* 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 <christophe.romain@process-one.net>
 
 	* src/mod_pubsub/mod_pubsub.erl: Allow to get subscriptions on a given

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,9 +94,22 @@ 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=waiting}] ->
+	    timer:sleep(2000),
+	    get_caps({U, S, R}, Retry-1);
 	[#user_caps{caps=Caps}] ->
 	    Caps;
 	_ ->
@@ -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),            % clean in case of explicitely set to disc_copies
-    mnesia:clear_table(user_caps_resources),
+    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)}),
+    mnesia:transaction(fun() ->
-    case ejabberd_sm:get_user_resources(U, S) of
+	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),
+		% only store resource of caps aware external contacts
-	    mnesia:dirty_write(#user_caps_resources{uid = BUID, resource = list_to_binary(R)});
+		BUID = exmpp_jid:bare_jid_to_binary(From),
-	_ -> 
+		mnesia:dirty_write(#user_caps_resources{uid = BUID, resource = list_to_binary(R)});
-	    ok
+	    _ -> 
-    end,
+		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) ->

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}) ->
+	%% get_caps may be blocked few seconds, get_caps as well
-		    case mod_caps:get_caps({User, Server, Resource}) of
+	%% so we spawn the whole process not to block other queries
-		    nothing -> {R, L};
+	spawn(fun() ->
-		    Caps -> {true, [{Resource, Caps} | L]}
+	    {HasCaps, ResourcesCaps} = lists:foldl(fun(Resource, {R, L}) ->
-		    end
+			case mod_caps:get_caps({User, Server, Resource}) of
-		end, {false, []}, Resources),
+			nothing -> {R, L};
-	case HasCaps of
+			Caps -> {true, [{Resource, Caps} | L]}
-	    true ->
+			end
-		Host = State#state.host,
+		    end, {false, []}, Resources),
-		ServerHost = State#state.server_host,
+	    case HasCaps of
-		Owner = jlib:short_prepd_bare_jid(JID),
+		true ->
-		lists:foreach(fun(#pubsub_node{nodeid = {_, Node}, type = Type, id = NodeId, options = Options}) ->
+		    Host = State#state.host,
-		    case get_option(Options, send_last_published_item) of
+		    ServerHost = State#state.server_host,
-			on_sub_and_presence ->
+		    Owner = jlib:short_prepd_bare_jid(JID),
-			    lists:foreach(fun({Resource, Caps}) ->
+		    lists:foreach(fun(#pubsub_node{nodeid = {_, Node}, type = Type, id = NodeId, options = Options}) ->
-				CapsNotify = case catch mod_caps:get_features(ServerHost, Caps) of
+			case get_option(Options, send_last_published_item) of
-					Features when is_list(Features) -> lists:member(Node ++ "+notify", Features);
+			    on_sub_and_presence ->
-					_ -> false
+				lists:foreach(fun({Resource, Caps}) ->
-				    end,
+				    CapsNotify = case catch mod_caps:get_features(ServerHost, Caps) of
-?INFO_MSG("notify ~s ~s",[Node, CapsNotify]),
+					    Features when is_list(Features) -> lists:member(Node ++ "+notify", Features);
-				case CapsNotify of
+					    _ -> false
-				    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 ->
+				    case CapsNotify of
-					    send_items(Owner, Node, NodeId, Type, LJID, last);
 					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;
+				    end
-				    false ->
+				end, ResourcesCaps);
-					ok
+			    _ ->
-				end
+				ok
-			    end, ResourcesCaps);
+			end
-			_ ->
+		    end, tree_action(Host, get_nodes, [Owner, JID]));
-			    ok
+		false ->
-		    end
+		    ok
-		end, tree_action(Host, get_nodes, [Owner, JID]));
+	    end
-	    false ->
+	end),
-		ok
-	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),