OTP-8431: Fix error handling of httpc_manager and its starter process

OTP-8431: [email protected]

6 files changed, 121 insertions, 34 deletions

diff --git a/lib/inets/doc/src/httpc.xml b/lib/inets/doc/src/httpc.xml
index e143ba2c1a..ca93190f61 100644
--- a/lib/inets/doc/src/httpc.xml
+++ b/lib/inets/doc/src/httpc.xml
@@ -277,7 +277,7 @@ ssl_options() = {verify,     code()}   |
 	      stream_start, Headers}, {http, {RequestId, stream,
 	      BinBodyPart}, {http, {RequestId, stream_end, Headers}. When
 	      streaming to to the calling processes using the option
-	      <c>{self once}</c> the first message will have an additional
+	      <c>{self, once}</c> the first message will have an additional
 	      element e.i. {http, {RequestId, stream_start, Headers, Pid},
 	      this is the process id that should be used as an argument to
 	      http:stream_next/1 to trigger the next message to be sent to
@@ -329,7 +329,7 @@ ssl_options() = {verify,     code()}   |
             <p>Note that the validity of the options are <em>not</em>
               checked in any way. </p>
             <p>Note that this may change the socket behaviour 
-              (see <seealso marker="inet#setopts">inet:setopts/2</seealso>) 
+              (see <seealso marker="kernel:inet#setopts">inet:setopts/2</seealso>) 
               for an already existing, and therefor already connected 
               request handler. </p>
             <p>By defaults the socket options set by the 
diff --git a/lib/inets/doc/src/notes.xml b/lib/inets/doc/src/notes.xml
index e95c8d6e97..0c524f00d1 100644
--- a/lib/inets/doc/src/notes.xml
+++ b/lib/inets/doc/src/notes.xml
@@ -354,7 +354,7 @@
               request, when the client connects to the server. Default 
               value is that of the <c>timeout</c> option. </p>
             <p>See the 
-	      <seealso marker="http#request2">request/4,5</seealso> 
+	      <seealso marker="httpc#request2">request/4,5</seealso> 
               function for more info. </p>
 	    <p>Own Id: OTP-7298</p>
 	    <!-- <p>Aux Id: seq11086</p> -->
@@ -461,7 +461,7 @@
               the client connects to the server. </p>
 	    <p>As a side-effect of this, the option <c>ipv6</c> has been 
               removed and replaced by the <c>ipfamily</c> option. </p>
-	    <p>See <seealso marker="http#set_options">http:set_options/1,2</seealso> 
+	    <p>See <seealso marker="httpc#set_options">http:set_options/1,2</seealso> 
               for more info. </p>
             <p>*** POTENTIAL INCOMPATIBILITY ***</p>
 	    <p>Own Id: OTP-8004</p>
diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl
index fec74932a2..31585537d4 100644
--- a/lib/inets/src/http_client/httpc_handler.erl
+++ b/lib/inets/src/http_client/httpc_handler.erl
@@ -267,9 +267,14 @@ handle_call({connect_and_send, #request{address = Address0,
 	    %%send_ssl_tunnel_request(Address, Request,
 	    %%		    #state{options = Options,
 	    %%		   status = ssl_tunnel});
-	    Reason = https_through_proxy_is_not_currently_supported,
-	    Error  = {error, Reason},
-	    {stop, Error, Error, State};
+	    Reason = {failed_connecting, 
+		      https_through_proxy_is_not_currently_supported},
+	    %% Send a reply to the original caller
+	    ErrorResponse = httpc_response:error(Request, Reason), 
+	    httpc_response:send(Request#request.from, ErrorResponse),
+	    %% Reply to the manager
+	    ErrorReply    = {error, Reason},
+	    {stop, normal, ErrorReply, State};
 	true ->
 	    case connect_and_send_first_request(Address, Request, State) of
 		{ok, NewState} ->
diff --git a/lib/inets/src/http_client/httpc_manager.erl b/lib/inets/src/http_client/httpc_manager.erl
index f8fc6322ed..f3cd81f4a7 100644
--- a/lib/inets/src/http_client/httpc_manager.erl
+++ b/lib/inets/src/http_client/httpc_manager.erl
@@ -62,7 +62,7 @@
 	  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 | operational | canceled
+	  state    % State of the handler: initiating | started | operational | canceled
 	 }).
 
 %% Entries in the handler / request cross-ref table
@@ -539,6 +539,11 @@ handle_cast(Msg, #state{profile_name = ProfileName} = State) ->
 %%          {stop, Reason, State}            (terminate/2 is called)
 %% Description: Handling all non call/cast messages
 %%---------------------------------------------------------
+
+handle_info({started, StarterPid, ReqId, HandlerPid}, State) ->
+    handle_started(StarterPid, ReqId, HandlerPid, State),
+    {noreply, State};
+
 handle_info({connect_and_send, StarterPid, ReqId, HandlerPid, Res}, State) ->
     handle_connect_and_send(StarterPid, ReqId, HandlerPid, Res, State),
     {noreply, State};
@@ -633,6 +638,38 @@ get_handler_info(Tab) ->
 
 %% 
 %% The request handler process is started asynchronously by a 
+%% "starter process". When the handler has sucessfully been started,
+%% this message (started) is sent.
+%% 
+
+handle_started(StarterPid, ReqId, HandlerPid, 
+			#state{profile_name = Profile, 
+			       handler_db   = HandlerDb}) ->
+    case ets:lookup(HandlerDb, ReqId) of
+	[#handler_info{state = initiating} = HandlerInfo] ->
+	    ?hcri("received started ack for initiating handler", []),
+	    HandlerInfo2 = HandlerInfo#handler_info{handler = HandlerPid,
+						    state   = started}, 
+	    ets:insert(HandlerDb, HandlerInfo2),
+	    ok;
+
+	[#handler_info{state = State}] ->
+	    error_report(Profile, 
+			 "unexpected (started) message for handler (~p) in state "
+			 "~p regarding request ~p - ignoring", [HandlerPid, State, ReqId]),
+	    ?hcri("received unexpected started message", [{state, State}]),
+	    ok;
+
+	[] ->
+	    error_report(Profile, 
+			 "unknown handler ~p (~p) started for request ~w - canceling",
+			 [HandlerPid, StarterPid, ReqId]),
+	    httpc_handler:cancel(ReqId, HandlerPid)
+    end.
+
+
+%% 
+%% The request handler process is started asynchronously by a 
 %% "starter process". When that process terminates it sends 
 %% one of two messages. These ara handled by the two functions
 %% below.
@@ -642,8 +679,8 @@ handle_connect_and_send(_StarterPid, ReqId, HandlerPid, Result,
 			#state{profile_name = Profile, 
 			       handler_db   = HandlerDb}) ->
     case ets:lookup(HandlerDb, ReqId) of
-	[#handler_info{state = initiating} = HandlerInfo] when Result =:= ok ->
-	    ?hcri("received connect-and-send ack for initiating handler", []),
+	[#handler_info{state = started} = HandlerInfo] when Result =:= ok ->
+	    ?hcri("received connect-and-send ack for started handler", []),
 	    HandlerInfo2 = HandlerInfo#handler_info{starter = undefined, 
 						    handler = HandlerPid,
 						    state   = operational}, 
@@ -658,28 +695,24 @@ handle_connect_and_send(_StarterPid, ReqId, HandlerPid, Result,
 	    ets:insert(HandlerDb, HandlerInfo2),
 	    ok;
 
-	[#handler_info{from = From}] ->
+	[#handler_info{state = State}] when Result =/= ok ->
 	    error_report(Profile, 
-			 "handler (~p) failed to connect and/or "
+			 "handler (~p, ~w) failed to connect and/or "
 			 "send request ~p"
-			 "~n   Error: ~p", [HandlerPid, ReqId, Result]),
-	    ?hcri("received connect-and-send error", [{result, Result}]),
-	    Reason2 = 
-		case Result of
-		    {error, Reason} ->
-			{failed_connecting, Reason};
-		    _ ->
-			{failed_connecting, Result}
-		end,
-	    DummyReq = #request{id = ReqId}, 
-	    httpc_response:send(From, httpc_response:error(DummyReq, Reason2)),
-	    %% gen_server:reply(From, Error),
+			 "~n   Result: ~p", 
+			 [HandlerPid, State, ReqId, Result]),
+	    ?hcri("received connect-and-send error", 
+		  [{result, Result}, {state, State}]),
+	    %% We don't need to send a response to the original caller
+	    %% because the handler already sent one in its terminate
+	    %% function.
 	    ets:delete(HandlerDb, ReqId), 
 	    ok;
 
 	[] ->
 	    error_report(Profile, 
-			 "handler successfully (~p) started for unknown request ~p",
+			 "handler (~p) successfully started "
+			 "for unknown request ~p => canceling",
 			 [HandlerPid, ReqId]),
 	    httpc_handler:cancel(ReqId, HandlerPid)
     end.
@@ -689,13 +722,11 @@ handle_failed_starting_handler(_StarterPid, ReqId, Error,
 			       #state{profile_name = Profile, 
 				      handler_db = HandlerDb}) ->
     case ets:lookup(HandlerDb, ReqId) of
-	[#handler_info{state = canceled, 
-		       from  = From}] ->
+	[#handler_info{state = canceled}] ->
 	    error_report(Profile, 
 			 "failed starting handler for request ~p"
 			 "~n   Error: ~p", [ReqId, Error]),
 	    request_canceled(Profile, ReqId), % Fake signal from handler
-	    gen_server:reply(From, Error),
 	    ets:delete(HandlerDb, ReqId), 
 	    ok;
 
@@ -703,7 +734,15 @@ handle_failed_starting_handler(_StarterPid, ReqId, Error,
 	    error_report(Profile, 
 			 "failed starting handler for request ~p"
 			 "~n   Error: ~p", [ReqId, Error]),
-	    gen_server:reply(From, Error),
+	    Reason2 = 
+		case Error of
+		    {error, Reason} ->
+			{failed_connecting, Reason};
+		    _ ->
+			{failed_connecting, Error}
+		end,
+	    DummyReq = #request{id = ReqId}, 
+	    httpc_response:send(From, httpc_response:error(DummyReq, Reason2)),
 	    ets:delete(HandlerDb, ReqId), 
 	    ok;
 
@@ -719,10 +758,32 @@ maybe_handle_terminating_starter(MeybeStarterPid, Reason, HandlerDb) ->
     Pattern = #handler_info{starter = MeybeStarterPid, _ = '_'}, 
     case ets:match_object(HandlerDb, Pattern) of
 	[#handler_info{id = ReqId, from = From, state = initiating}] ->
-	    Error = {error, {failed_starting_request_handler, Reason}}, 
-	    gen_server:reply(From, Error),
+	    %% The starter process crashed before it could start the 
+	    %% the handler process, therefor we need to answer the 
+	    %% original caller.
+	    ?hcri("starter process crashed bfore starting handler", 
+		  [{starter, MeybeStarterPid}, {reason, Reason}]),
+	    Reason2 = 
+		case Reason of
+		    {error, Error} ->
+			{failed_connecting, Error};
+		    _ ->
+			{failed_connecting, Reason}
+		end,
+	    DummyReq = #request{id = ReqId}, 
+	    httpc_response:send(From, httpc_response:error(DummyReq, Reason2)),
 	    ets:delete(HandlerDb, ReqId),
 	    ok;
+
+	[#handler_info{state = State} = HandlerInfo] ->
+	    %% The starter process crashed after the handler was started. 
+	    %% The handler will answer to the original caller.
+	    ?hcri("starter process crashed after starting handler", 
+		  [{starter, MeybeStarterPid}, {reason, Reason}, {state, State}]),
+	    HandlerInfo2 = HandlerInfo#handler_info{starter = undefined}, 
+	    ets:insert(HandlerDb, HandlerInfo2),
+	    ok;
+
 	_ ->
 	    ok
     end.
@@ -886,6 +947,9 @@ create_handler_starter(#request{id          = Id,
 		      [{id, Id}, {profile, ProfileName}, {result, Result1}]),
 		case Result1 of
 		    {ok, HandlerPid} ->
+			StartedMessage = 
+			    {started, self(), Id, HandlerPid}, 
+			ManagerPid ! StartedMessage, 
 			Result2 = httpc_handler:connect_and_send(Request, 
 								 HandlerPid),
 			?hcrd("handler starter - connected and sent", 
diff --git a/lib/inets/test/httpc_SUITE.erl b/lib/inets/test/httpc_SUITE.erl
index 96099c49fd..aa65fb1197 100644
--- a/lib/inets/test/httpc_SUITE.erl
+++ b/lib/inets/test/httpc_SUITE.erl
@@ -1440,13 +1440,31 @@ proxy_page_does_not_exist(Config) when is_list(Config) ->
 
 
 %%-------------------------------------------------------------------------
+
 proxy_https_not_supported(doc) ->
     [];
 proxy_https_not_supported(suite) ->
     [];
 proxy_https_not_supported(Config) when is_list(Config) ->
-    {error, {failed_connecting, https_through_proxy_is_not_currently_supported}} = 
-	http:request(get, {"https://login.yahoo.com", []}, [], []),
+    Result = http:request(get, {"https://login.yahoo.com", []}, [], []),
+    case Result of
+	{error, Reason} ->
+	    %% ok so far
+	    case Reason of
+		{failed_connecting, Why} ->
+		    %% ok, now check why
+		    case Why of
+			https_through_proxy_is_not_currently_supported ->
+			    ok;
+			_ ->
+			    tsf({unexpected_why, Why})
+		    end;
+		_ ->
+		    tsf({unexpected_reason, Reason})
+	    end;
+	_ ->
+	    tsf({unexpected_result, Result})
+    end,
     ok.
 
 
diff --git a/lib/inets/vsn.mk b/lib/inets/vsn.mk
index 746517eed5..c905532066 100644
--- a/lib/inets/vsn.mk
+++ b/lib/inets/vsn.mk
@@ -19,7 +19,7 @@
 
 APPLICATION = inets
 INETS_VSN   = 5.3
-PRE_VSN     =-p13
+PRE_VSN     =-p14
 APP_VSN     = "$(APPLICATION)-$(INETS_VSN)$(PRE_VSN)"
 
 TICKETS = \