[inets/httpc] Cancel request does not work

Cancel request does not work due to incorrect handler table creation
(wrong keypos).

OTP-10092

3 files changed, 154 insertions, 132 deletions

diff --git a/lib/inets/src/http_client/httpc_manager.erl b/lib/inets/src/http_client/httpc_manager.erl
index b225b43214..771968e169 100644
--- a/lib/inets/src/http_client/httpc_manager.erl
+++ b/lib/inets/src/http_client/httpc_manager.erl
@@ -59,17 +59,9 @@
 	  options = #options{}
 	 }).
 
--record(handler_info, 
-	{
-	  id,      % Id of the request:          request_id()
-	  starter, % Pid of the handler starter process (temp): pid()
-	  handler, % Pid of the handler process: pid()
-	  from,    % From for the request:  from()
-	  state    % State of the handler: initiating | started | operational | canceled
-	 }).
-
 -define(DELAY, 500).
 
+
 %%====================================================================
 %% Internal Application API
 %%====================================================================
@@ -379,8 +371,7 @@ do_init(ProfileName, CookiesDir) ->
     %% Create handler db
     ?hcrt("create handler/request db", []),
     HandlerDbName = handler_db_name(ProfileName), 
-    ets:new(HandlerDbName, 
-	    [protected, set, named_table, {keypos, #handler_info.id}]),
+    ets:new(HandlerDbName, [protected, set, named_table, {keypos, 1}]),
 
     %% Cookie DB
     ?hcrt("create cookie db", []),
@@ -414,9 +405,10 @@ handle_call({request, Request}, _, State) ->
 	    {stop, Error, httpc_response:error(Request, Error), State}
     end;
 
-handle_call({cancel_request, RequestId}, From, State) ->
+handle_call({cancel_request, RequestId}, From, 
+	    #state{handler_db = HandlerDb} = State) ->
     ?hcri("cancel_request", [{request_id, RequestId}]),
-    case ets:lookup(State#state.handler_db, RequestId) of
+    case ets:lookup(HandlerDb, RequestId) of
 	[] ->
 	    %% The request has allready compleated make sure
 	    %% it is deliverd to the client process queue so
@@ -428,9 +420,9 @@ handle_call({cancel_request, RequestId}, From, State) ->
 	    {noreply, State};
 	[{_, Pid, _}] ->
 	    httpc_handler:cancel(RequestId, Pid, From),
-	    {noreply, State#state{cancel = 
-				  [{RequestId, Pid, From} |
-				   State#state.cancel]}}
+	    {noreply, 
+	     State#state{cancel = 
+			 [{RequestId, Pid, From} | State#state.cancel]}}
     end;
 
 handle_call(reset_cookies, _, #state{cookie_db = CookieDb} = State) ->
@@ -645,7 +637,7 @@ code_change(_,
 code_change(_, State, _) ->
     {ok, State}.
 
-%% This function is to catch everything that calls through the cracks...
+%% This function is used to catch everything that falls through the cracks...
 update_session_table(SessionDB, Transform) ->
     ets:safe_fixtable(SessionDB, true),
     update_session_table(SessionDB, ets:first(SessionDB), Transform),
@@ -678,35 +670,42 @@ get_manager_info(#state{handler_db = HDB,
     CookieInfo  = httpc_cookie:which_cookies(CDB),
     [{handlers, HandlerInfo}, {cookies, CookieInfo}].
 
+sort_handlers(Unsorted) ->
+    sort_handlers2(lists:keysort(1, Unsorted)).
+
+sort_handlers2([]) ->
+    [];
+sort_handlers2([{HandlerPid, RequestId}|L]) ->
+    {Handler, Rest} = sort_handlers2(HandlerPid, [RequestId], L),
+    [Handler | sort_handlers2(Rest)].
+
+sort_handlers2(HandlerPid, Reqs, []) ->
+    {{HandlerPid, lists:sort(Reqs)}, []};
+sort_handlers2(HandlerPid, Reqs, [{HandlerPid, ReqId}|Rest]) ->
+    sort_handlers2(HandlerPid, [ReqId|Reqs], Rest);
+sort_handlers2(HandlerPid1, Reqs, [{HandlerPid2, _}|_] = Rest) 
+  when HandlerPid1 =/= HandlerPid2 ->
+    {{HandlerPid1, lists:sort(Reqs)}, Rest}.
+
 get_handler_info(Tab) ->
-    Pattern = #handler_info{handler = '$1',
-			    state   = '$2', 
-			    _ = '_'},
-    Handlers1 = [{Pid, State} || [Pid, State] <- ets:match(Tab, Pattern)],
-    F = fun({Pid, State} = Elem, Acc) when State =/= canceled -> 
-		case lists:keymember(Pid, 1, Acc) of
-		    true ->
-			Acc;
-		    false ->
-			[Elem | Acc]
-		end;
-	   (_, Acc) ->
-		Acc
-	end,
-    Handlers2 = lists:foldl(F, [], Handlers1),
-    Handlers3 = [{Pid, State, 
-		  case (catch httpc_handler:info(Pid)) of
-		      {'EXIT', _} -> 
+    Pattern   = {'$2', '$1', '_'},
+    Handlers1 = [{Pid, Id} || [Pid, Id] <- ets:match(Tab, Pattern)],
+    Handlers2 = sort_handlers(Handlers1), 
+    Handlers3 = [{Pid, Reqs, 
+		  try
+		      begin
+			  httpc_handler:info(Pid)
+		      end
+		  catch
+		      _:_ ->
 			  %% Why would this crash? 
 			  %% Only if the process has died, but we don't 
 			  %% know about it?
-			  [];
-		      Else ->
-			  Else
-		  end} || 
-		    {Pid, State} <- Handlers2],
+			  []
+		  end} || {Pid, Reqs} <- Handlers2],
     Handlers3.
 
+
 handle_request(#request{settings = 
 			#http_options{version = "HTTP/0.9"}} = Request,
 	       State) ->
@@ -758,19 +757,21 @@ handle_request(Request, State = #state{options = Options}) ->
     {reply, {ok, NewRequest#request.id}, State}.
 
 
-start_handler(Request, State) ->
+start_handler(#request{id   = Id, 
+		       from = From} = Request, 
+	      #state{profile_name = ProfileName, 
+		     handler_db   = HandlerDb, 
+		     options      = Options}) ->
     {ok, Pid} =
 	case is_inets_manager() of
 	    true ->
 		httpc_handler_sup:start_child([whereis(httpc_handler_sup),
-					       Request, State#state.options,
-					       State#state.profile_name]);
+					       Request, Options, ProfileName]);
 	    false ->
-		httpc_handler:start_link(self(), Request, State#state.options,
-					 State#state.profile_name)
+		httpc_handler:start_link(self(), Request, Options, ProfileName)
 	end,
-    ets:insert(State#state.handler_db, {Request#request.id,
-					Pid, Request#request.from}),
+    HandlerInfo = {Id, Pid, From}, 
+    ets:insert(HandlerDb, HandlerInfo), 
     erlang:monitor(process, Pid).
 
 
@@ -827,12 +828,14 @@ select_session(Candidates, Max) ->
 	    {ok, HandlerPid}
     end.
 
-pipeline_or_keep_alive(Request, HandlerPid, State) ->
+pipeline_or_keep_alive(#request{id   = Id, 
+				from = From} = Request, 
+		       HandlerPid, 
+		       #state{handler_db = HandlerDb} = State) ->
     case (catch httpc_handler:send(Request, HandlerPid)) of
 	ok ->
-	    ets:insert(State#state.handler_db, {Request#request.id,
-						HandlerPid,
-						Request#request.from});
+	    HandlerInfo = {Id, HandlerPid, From}, 
+	    ets:insert(HandlerDb, HandlerInfo);
 	_  -> % timeout pipelining failed
 	    start_handler(Request, State)
     end.
diff --git a/lib/inets/test/httpc_SUITE.erl b/lib/inets/test/httpc_SUITE.erl
index c4943cbb4c..1cdd96f0b0 100644
--- a/lib/inets/test/httpc_SUITE.erl
+++ b/lib/inets/test/httpc_SUITE.erl
@@ -768,120 +768,83 @@ test_pipeline(URL) ->
     
     p("test_pipeline -> issue (async) request 1"
       "~n   when profile info: ~p", [httpc:info()]),
-    {ok, RequestId1} =
+    {ok, RequestIdA1} =
 	httpc:request(get, {URL, []}, [], [{sync, false}]),
-    tsp("RequestId1: ~p", [RequestId1]),
-    p("test_pipeline -> RequestId1: ~p"
-      "~n   when profile info: ~p", [RequestId1, httpc:info()]),
+    tsp("RequestIdA1: ~p", [RequestIdA1]),
+    p("test_pipeline -> RequestIdA1: ~p"
+      "~n   when profile info: ~p", [RequestIdA1, httpc:info()]),
     
     %% Make sure pipeline is initiated
     p("test_pipeline -> sleep some", []),
     test_server:sleep(4000),
     
-    p("test_pipeline -> issue (async) request 2"
+    p("test_pipeline -> issue (async) request A2, A3 and A4"
       "~n   when profile info: ~p", [httpc:info()]),
-    {ok, RequestId2} =
+    {ok, RequestIdA2} =
 	httpc:request(get, {URL, []}, [], [{sync, false}]),
-    tsp("RequestId2: ~p", [RequestId2]),
-    p("test_pipeline -> RequestId2: ~p"
-      "~n   when profile info: ~p", [RequestId2, httpc:info()]),
+    {ok, RequestIdA3} =
+	httpc:request(get, {URL, []}, [], [{sync, false}]),
+    {ok, RequestIdA4} =
+	httpc:request(get, {URL, []}, [], [{sync, false}]),
+    tsp("RequestIdAs => A2: ~p, A3: ~p and A4: ~p", 
+	[RequestIdA2, RequestIdA3, RequestIdA4]),
+    p("test_pipeline -> RequestIds => A2: ~p, A3: ~p and A4: ~p"
+      "~n   when profile info: ~p", 
+      [RequestIdA2, RequestIdA3, RequestIdA4, httpc:info()]),
     
     p("test_pipeline -> issue (sync) request 3"),
     {ok, {{_,200,_}, [_ | _], [_ | _]}} =
 	httpc:request(get, {URL, []}, [], []),
     
-    p("test_pipeline -> expect reply for (async) request 1 or 2"
+    p("test_pipeline -> expect reply for (async) request A1, A2, A3 and A4"
       "~n   when profile info: ~p", [httpc:info()]),
-    receive
-	{http, {RequestId1, {{_, 200, _}, _, _}}} ->
-	    p("test_pipeline -> "
-	      "received reply for (async) request 1 - now wait for 2"
-	      "~n   when profile info: ~p", [httpc:info()]),
-	    receive
-		{http, {RequestId2, {{_, 200, _}, _, _}}} ->
-		    p("test_pipeline -> "
-		      "received reply for (async) request 2"
-		      "~n   when profile info: ~p", [httpc:info()]),
-		    ok;
-		{http, Msg1} ->
-		    tsf(Msg1)
-	    end;
-	{http, {RequestId2, {{_, 200, _}, _, _}}} ->
-	    io:format("test_pipeline -> "
-		      "received reply for (async) request 2 - now wait for 1"
-		      "~n   when profile info: ~p", [httpc:info()]),
-	    receive
-		{http, {RequestId1, {{_, 200, _}, _, _}}} ->
-		    io:format("test_pipeline -> "
-			      "received reply for (async) request 1"
-			      "~n   when profile info: ~p", [httpc:info()]),
-		    ok;
-		{http, Msg2} ->
-		    tsf(Msg2)
-	    end;
-	{http, Msg3} ->
-	    tsf(Msg3)
-    after 60000 ->
-	    receive Any1 ->
-		    tsp("received crap after timeout: ~n   ~p", [Any1]),
-		    tsf({error, {timeout, Any1}})
-	    end
-    end,
-    
+    pipeline_await_async_reply([{RequestIdA1, a1, 200}, 
+				{RequestIdA2, a2, 200}, 
+				{RequestIdA3, a3, 200}, 
+				{RequestIdA4, a4, 200}], ?MINS(1)),
+
     p("test_pipeline -> sleep some"
       "~n   when profile info: ~p", [httpc:info()]),
     test_server:sleep(4000),
     
-    p("test_pipeline -> issue (async) request 4"
+    p("test_pipeline -> issue (async) request B1, B2, B3 and B4"
       "~n   when profile info: ~p", [httpc:info()]),
-    {ok, RequestId3} =
+    {ok, RequestIdB1} =
 	httpc:request(get, {URL, []}, [], [{sync, false}]),
-    tsp("RequestId3: ~p", [RequestId3]),
-    p("test_pipeline -> RequestId3: ~p"
-      "~n   when profile info: ~p", [RequestId3, httpc:info()]),
-    
-    p("test_pipeline -> issue (async) request 5"
-      "~n   when profile info: ~p", [httpc:info()]),
-    {ok, RequestId4} =
+    {ok, RequestIdB2} =
+	httpc:request(get, {URL, []}, [], [{sync, false}]),
+    {ok, RequestIdB3} =
 	httpc:request(get, {URL, []}, [], [{sync, false}]),
-    tsp("RequestId4: ~p", [RequestId4]),
-    p("test_pipeline -> RequestId4: ~p"
-      "~n   when profile info: ~p", [RequestId4, httpc:info()]),
+    {ok, RequestIdB4} =
+	httpc:request(get, {URL, []}, [], [{sync, false}]),
+    tsp("RequestIdBs => B1: ~p, B2: ~p, B3: ~p and B4: ~p", 
+	[RequestIdB1, RequestIdB2, RequestIdB3, RequestIdB4]),
+    p("test_pipeline -> RequestIdBs => B1: ~p, B2: ~p, B3: ~p and B4: ~p"
+      "~n   when profile info: ~p", 
+      [RequestIdB1, RequestIdB2, RequestIdB3, RequestIdB4, httpc:info()]),
     
-    p("test_pipeline -> cancel (async) request 4"
+    p("test_pipeline -> cancel (async) request B2"
       "~n   when profile info: ~p", [httpc:info()]),
-    ok = httpc:cancel_request(RequestId3),
+    ok = httpc:cancel_request(RequestIdB2),
     
     p("test_pipeline -> "
-      "expect *no* reply for cancelled (async) request 4 (for 3 secs)"
+      "expect *no* reply for cancelled (async) request B2 (for 3 secs)"
       "~n   when profile info: ~p", [httpc:info()]),
     receive
-	{http, {RequestId3, _}} ->
+	{http, {RequestIdB2, _}} ->
 	    tsf(http_cancel_request_failed)
     after 3000 ->
 	    ok
     end,
     
-    p("test_pipeline -> expect reply for (async) request 4"
+    p("test_pipeline -> expect reply for (async) request B1, B3 and B4"
       "~n   when profile info: ~p", [httpc:info()]),
-    Body =
-	receive
-	    {http, {RequestId4, {{_, 200, _}, _, BinBody4}}} = Res ->
-		p("test_pipeline -> "
-		  "received reply for (async) request 5"
-		  "~n   when profile info: ~p", [httpc:info()]),
-		tsp("Receive : ~p", [Res]),
-		BinBody4;
-	    {http, Msg4} ->
-		tsf(Msg4)
-	after 60000 ->
-		receive Any2 ->
-			tsp("received crap after timeout: ~n   ~p", [Any2]),
-			tsf({error, {timeout, Any2}})
-		end
-	end,
-    
-    p("test_pipeline -> check reply for (async) request 5"
+    Bodies = pipeline_await_async_reply([{RequestIdB1, b1, 200}, 
+					 {RequestIdB3, b3, 200}, 
+					 {RequestIdB4, b4, 200}], ?MINS(1)),
+    [{b1, Body}|_] = Bodies, 
+
+    p("test_pipeline -> check reply for (async) request B1"
       "~n   when profile info: ~p", [httpc:info()]),
     inets_test_lib:check_body(binary_to_list(Body)),
     
@@ -898,6 +861,58 @@ test_pipeline(URL) ->
       "~n   when profile info: ~p", [httpc:info()]),
     ok.
 
+pipeline_await_async_reply(ReqIds, Timeout) ->
+    pipeline_await_async_reply(ReqIds, Timeout, []).
+
+pipeline_await_async_reply([], _, Acc) ->
+    lists:keysort(1, Acc);
+pipeline_await_async_reply(ReqIds, Timeout, Acc) when Timeout > 0 ->
+    T1 = inets_test_lib:timestamp(), 
+    p("pipeline_await_async_reply -> await replies"
+      "~n   ReqIds:  ~p"
+      "~n   Timeout: ~p", [ReqIds, Timeout]),
+    receive
+	{http, {RequestId, {{_, Status, _}, _, Body}}} ->
+	    p("pipeline_await_async_reply -> received reply for"
+	      "~n   RequestId: ~p"
+	      "~n   Status:    ~p", [RequestId, Status]),
+	    case lists:keysearch(RequestId, 1, ReqIds) of
+		{value, {RequestId, N, Status}} ->
+		    p("pipeline_await_async_reply -> "
+		      "found expected request ~w", [N]),
+		    ReqIds2 = lists:keydelete(RequestId, 1, ReqIds),
+		    NewTimeout = Timeout - (inets_test_lib:timestamp()-T1), 
+		    pipeline_await_async_reply(ReqIds2, NewTimeout, 
+					      [{N, Body} | Acc]);
+		{value, {RequestId, N, WrongStatus}} ->
+		    p("pipeline_await_async_reply -> "
+		      "found request ~w with wrong status", [N]),
+		    tsf({reply_with_unexpected_status, 
+			 {RequestId, N, WrongStatus}});
+		false ->
+		    tsf({unexpected_reply, {RequestId, Status}})
+	    end;
+	{http, Msg} ->
+	    tsf({unexpected_reply, Msg})
+    after Timeout ->
+	    receive 
+		Any ->
+		    tsp("pipeline_await_async_reply -> "
+			"received unknown data after timeout: "
+			"~n   ~p", [Any]),
+		    tsf({timeout, {unknown, Any}})
+	    end
+    end;
+pipeline_await_async_reply(ReqIds, _, Acc) ->
+    tsp("pipeline_await_async_reply -> "
+	"timeout: "
+	"~n   ~p"
+	"~nwhen"
+	"~n   ~p", [ReqIds, Acc]),
+    tsf({timeout, ReqIds, Acc}).
+    
+
+    
 %%-------------------------------------------------------------------------
 http_trace(doc) ->
     ["Perform a TRACE request that goes through a proxy."];
diff --git a/lib/inets/test/inets_test_lib.erl b/lib/inets/test/inets_test_lib.erl
index c94be796cd..7f4b0ec8d8 100644
--- a/lib/inets/test/inets_test_lib.erl
+++ b/lib/inets/test/inets_test_lib.erl
@@ -31,6 +31,7 @@
 	 send/3, close/2]).
 -export([copy_file/3, copy_files/2, copy_dirs/2, del_dirs/1]).
 -export([info/4, log/4, debug/4, print/4]).
+-export([timestamp/0, formated_timestamp/0]).
 -export([tsp/1, tsp/2, tsf/1, tss/1]).
 -export([check_body/1]).
 -export([millis/0, millis_diff/2, hours/1, minutes/1, seconds/1, sleep/1]).
@@ -642,6 +643,9 @@ tsf(Reason) ->
 tss(Time) ->
     test_server:sleep(Time).
 
+timestamp() ->
+    http_util:timestamp().
+
 formated_timestamp() ->
     format_timestamp( os:timestamp() ).