ERL-473: httpc socket_closed_remotely error

[OTP-Maintainer]

Original reporter: lintingbin
Affected versions: OTP-19.0, R16B
Fixed in version: OTP-20.2
Component: inets
Migrated from: https://bugs.erlang.org/browse/ERL-473

---

The httpc module does not handle the "max" parameter of the "keep-alive" in the http protocol.
!test.png|thumbnail! 

!httpc_bug.png|thumbnail!
The reason is that when a httpc_handle process is reach the max times of the "keep-alive:max", then server will close the connect, but httpc_manager also select this httpc_handler(because it is alive, and will append to its http request queue) to send http request, so the requester will receive the socket_closed_remotely.
!httpc_manager.png|thumbnail!

[OTP-Maintainer]

ingela said:

Would the following patch solve your problem? 

{code:java}
diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl
index 6907bf5..1482f4f 100644
--- a/lib/inets/src/http_client/httpc_handler.erl
+++ b/lib/inets/src/http_client/httpc_handler.erl
@@ -109,7 +109,7 @@ start_link(Parent, Request, Options, ProfileName) ->
 %% to be called by the httpc manager process.
 %%--------------------------------------------------------------------
 send(Request, Pid) ->
-    call(Request, Pid, 5000).
+    call(Request, Pid).
 
 
 %%--------------------------------------------------------------------
@@ -712,12 +712,16 @@ do_handle_info({'EXIT', _, _}, State = #state{request = undefined}) ->
 do_handle_info({'EXIT', _, _}, State) ->
     {noreply, State#state{status = close}}.
     
-
 call(Msg, Pid) ->
-    call(Msg, Pid, infinity).
-
-call(Msg, Pid, Timeout) ->
-    gen_server:call(Pid, Msg, Timeout).
+    try gen_server:call(Pid, Msg)
+    catch
+ 	exit:{noproc, _} ->
+ 	    {error, closed};
+	exit:{normal, _} ->
+	    {error, closed};
+	exit:{{shutdown, _},_} ->
+	    {error, closed}
+    end.
 
 cast(Msg, Pid) ->
     gen_server:cast(Pid, Msg).
diff --git a/lib/inets/src/http_client/httpc_manager.erl b/lib/inets/src/http_client/httpc_manager.erl
index a638644..ffdf160 100644
--- a/lib/inets/src/http_client/httpc_manager.erl
+++ b/lib/inets/src/http_client/httpc_manager.erl
@@ -849,11 +849,11 @@ pipeline_or_keep_alive(#request{id   = Id,
 				from = From} = Request, 
 		       HandlerPid, 
 		       #state{handler_db = HandlerDb} = State) ->
-    case (catch httpc_handler:send(Request, HandlerPid)) of
+    case httpc_handler:send(Request, HandlerPid) of
 	ok ->
 	    HandlerInfo = {Id, HandlerPid, From}, 
 	    ets:insert(HandlerDb, HandlerInfo);
-	_  -> % timeout pipelining failed
+	{error, closed}  -> % timeout pipelining failed
 	    start_handler(Request, State)
     end.
 
{code}

[OTP-Maintainer]

lintingbin said:

No, it's not the problem. @Ingela Anderton Andin.

[OTP-Maintainer]

ingela said:

Ah well, I think that it would improve the possibility of things not going wrong or working as intended. I think there are mechanism in place that should handle the problem the way I understand it from your description, not saying that they work. Do you think you could reproduce the error with say the http server in inets? It could speed things up as I am afraid this is not considered a top priority from the Ericsson point of view.

[OTP-Maintainer]

ingela said:

We need a test case to reproduce the erroneous behaviour. 

[OTP-Maintainer]

lintingbin said:

I use the apache server and set "MaxKeepAliveRequests 5" in the httpd.conf. The following is the bug test code:
-module(httpc_bug_test).

-export([test/0]).

test() ->
    test(0).

test(N) ->
    case httpc:request("http://localhost/test.php") of
        {ok, _} ->
            test(N + 1);
        Other ->
            {N, Other}
    end.

[OTP-Maintainer]

ingela said:

What dose test.php do? Does it matter? Could you use inets httpd server to set up the failing scenario? It would make a easier to handle regresseion test.

[OTP-Maintainer]

lintingbin said:

Ok, I test in an inets httpd web server.
*Step 1(start a web server):*
inets:start().
inets:start(httpd, [{port, 8080},{max_keep_alive_request, 5},{server_name,"httpd_test"}, {server_root,"."},{document_root,"."}, {bind_address, {0,0,0,0}}]). 
*Step 2:*
Place an empty index.html file in the current folder
*Step 3(run the bug test code):*
-module(httpc_bug_test).

-export([test/0]).

test() ->
    test(0).

test(N) ->
    case httpc:request("http://localhost:8080/index.html") of
        {ok, _} ->
            test(N + 1);
        Other ->
            {N, Other}
    end.
*Result:*
(all_in_one_33000@192.168.1.102)2> httpc_bug_test:test().
{36,{error,socket_closed_remotely}}
(all_in_one_33000@192.168.1.102)3> httpc_bug_test:test().
{6,{error,socket_closed_remotely}}

[OTP-Maintainer]

ingela said:

This seems to fix the problem. 


{code:java}
diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl
index 6907bf5..38d079a 100644
--- a/lib/inets/src/http_client/httpc_handler.erl
+++ b/lib/inets/src/http_client/httpc_handler.erl
@@ -340,9 +340,6 @@ terminate(normal,
     %% Cancel timers
     cancel_timers(Timers),
 
-    %% Maybe deliver answers to requests
-    deliver_answer(Request),
-
     %% And, just in case, close our side (**really** overkill)
     http_transport:close(SocketType, Socket);
 

{code}

[OTP-Maintainer]

lintingbin said:

This change will result in the failure of this request. This is not a good method.

[OTP-Maintainer]

ingela said:

Which request? 

I changed your test. And all requests are completed correctly with the patch above, but an error happens after 6 requests without it.

{code:java}

test() ->
    test(0).

test(100) ->
    ok;
test(N) ->
    case httpc:request("http://localhost:8080/index.html") of
        {ok, R} ->
            ct:pal("Result ~p", [{N, R}]),
            test(N + 1);
        Other ->
            {N, Other}
    end.

{code}

[OTP-Maintainer]

ingela said:

I had some more time to analyse this further. I was a little vague as I only had time to try your little example by hand, so I was not sure it was the correct solution, although I am not sure what you think should go wrong. However, I am not looking into that as at the moment as I think that the bug has actually been fixed on current maint branch by commit 05390c5a5a0b0657166d5c3c10e1a055cfe66e88

[OTP-Maintainer]

lintingbin said:

Where can I find this commit "05390c5a5a0b0657166d5c3c10e1a055cfe66e88"? 
I think the bug is that when a httpc_handle process is reach the max times of the "keep-alive:max", then server will close the connect, but httpc_manager also select this httpc_handler(because it is alive, and will append to its http request queue) to send http request, so the requester will receive the socket_closed_remotely.

[OTP-Maintainer]

ingela said:

The commit is on the maint branch.  When a request handler is closed it will retry requests that have not been compleated.  The manager process will make a call to the request handler and if the call succeeds the request will be retired by the close down mechanism if it fails a new handler will be started. I think that there might be a little glitch in this mechanism and that was my first patch which may still be relevant, but not the problem.  My second patch treats the symptom of "double close"
if the socket is remotely closed (only that case). When I first tested your use case I did not have the latest version of maint, and then I could reproduce it. When I was making it into a test case that can be automated and tried it on the latest maint I could no longer make it fail.  Why do you not try latest maint yourself? 

[OTP-Maintainer]

lintingbin said:

Thanks, I will try latest version httpc.

[OTP-Maintainer]

ingela said:

I have included the race condition elimination for 20.1, that was found due to this trouble report and has proven an improvement in our other tests. Though the problem reported was another one that has already been fixed by an other patch.

[OTP-Maintainer]

lintingbin said:

Eshell V9.1  (abort with ^G)
1> inets:start().
ok
2> httpc_bug_test:test().
{6,{error,socket_closed_remotely}}
3> httpc_bug_test:test().
{6,{error,socket_closed_remotely}}
4> httpc_bug_test:test().
{48,{error,socket_closed_remotely}}
5> httpc_bug_test:test().
{12,{error,socket_closed_remotely}}

It also has this problem when I test in OTP 20.1.

[OTP-Maintainer]

ingela said:

Sorry it should have been 20.2 the release yet to come. Please test on master branch. 

[OTP-Maintainer]

lintingbin said:

I have seen the code of master branch, which also deliver_answer "socket_closed_remotely" to the requester. I think that the problem isn't solved. I have a solution. Httpc_handler process doesn't deliver_answer "socket_closed_remotely" to the requester when http server close the connection, and start a new httpc_handler process for the requester.

deliver_answer(#request{from = From} = Request) 
  when From =/= answer_sent ->
    Response = httpc_response:error(Request, socket_closed_remotely),
    httpc_response:send(From, Response);
deliver_answer(_Request) ->
    ok.

[OTP-Maintainer]

ingela said:

Humm ... we will investigate again. However not prioritzed!

[OTP-Maintainer]

ingela said:

Alas we have been very busy with other issues and not had any more time to look into this, but now we got this PR 

https://github.com/erlang/otp/pull/1752

that might help this issue  could you please check?

[OTP-Maintainer]

ingela said:

I had the time to look at this again. The thing that is happening is that inets httpd does
what you configured it to do.

From the documentation

{max_keep_alive_request, integer()}

The number of requests that a client can do on one connection. When the server has responded to the number of requests defined by max_keep_alive_requests, the server closes the connection. The server closes it even if there are queued request. Default is no limit.

So if you remove the max_keep_aliva_request config so that there  is no limit the client will
never get the socket remotely closed. The reason why you do not get it deterministically is
it is very timing dependent if the client happens to reuse a keep-alive session or not. 

So that you get socket remotely closed sometimes with this set up is expected. 

[OTP-Maintainer]

lintingbin said:

It is a bug. Most of server have the max_keep_alive_request limit.

[OTP-Maintainer]

ingela said:

So what is the bug? Do you mean that the client should automatically retry the request? 

[OTP-Maintainer]

lintingbin said:

Other language don't have this problem. "retry the request" is a solution, but isn't a good solution.

[OTP-Maintainer]

ingela said:

But then I do not understand how you mean? Do you think it is the server that does the wrong thing? Does other servers treat the limit differently?  If the server closes the socket without sending the answer, automatically retrying the request is what can be done. That would be done for the client if it had used the pipeline approach instead of persistent connection approach (default).  Or did you expect just {error, closed} ? 

[OTP-Maintainer]

ingela said:

When you say other languages does not have this problem can you be more specific? I guess you mean http clients in other languages. And are you sure that those clients use persistent connections or pipe-lining? If the client sets up a new connection every time of course this will not happen. I looked at the code and I think that it could quite easily be made to in this particular case retry also the failing request pending requests will already be retried. We will consider this for a upcoming release. If anyone thinks this is an important feature PR are welcome.