[erlang-bugs] Revert commit 6189bc07

[chris]

> Date: Fri, 14 Feb 2014 10:11:26 +0100
> From: Ingela.Anderton.Andin@REDACTED
> Subject: Re: [erlang-bugs] Revert commit 6189bc07
> 
> Hi!
> 
> We will look into that, the commit was an attempt to improve the 
> utilization factor of pipelining. Here is the feedback from an open 
> source user that led us to to do this:
> 
> "The design of httpc_manager/_handler, wherein sessions are (a) 
> committed to the DB asynchronously, and (b) not committed until a 
> request is complete, results in max_sessions being grossly violated when 
> many requests are made in rapid succession.  This makes max_sessions, 
> and pipelining in general, nearly worthless -- rapid requests are 
> exactly the situation you want pipelining for!"

It is not possible to know whether pipelining or keep-alive connections are supported by the server until the first response from the server is received.  To be optimistic about this (since one can assume a client may know that the server actually does support such beforehand) then I could understand an option to force reusing an existing handler before the first response is received, but generally application code just deals with this by making a single request successfully before pipelining subsequent requests.  The original httpc code supported that methodology.

I also expect some tweaking of options to have helped with the issue from the above feedback -  increasing the value of max pipeline length option would have been a start for them.  But I also think that providing a max_connections option for preventing httpc from opening further non-persistent connections when max_sessions is hit would also be another approach.

> The thought was that the retry mechanism could allow us to have a more 
> optimistic approach to improve utilization maybe it was a bad 
> assumption. We will see what we can do about it.

Ok.  There may be something to address for looking up a session
 for subsequent pipeline requests after the initial one has been 
processed and set up, but that is not what this commit is doing.

In any case, if the commit won't be reverted (though it really should be), here are a couple of bugs that need fixed that it introduced:

The handler state session field is left uninitialized, resulting in crashes on subsequent requests before the first request response is handled and the state session field becomes initialized.  The Session created in the manager needs to be passed to the newly created handler as part of the init call, so it is part of the initial state.

In the manager, since sessions are inserted for every request now (ugh) keep-alive requests certainly should not try to use a session that is not actually persistent.  But assuming we are being optimistic for non-retry pipeline requests only, then this needs fixed for keep-alive types in the manager's select_session - ie the Pattern should force persistent = true for keep-alive as well. 

Regards,

Chris

> Regards Ingela Erlang/OTP Team - Ericsson AB
> 
> 
> On 02/07/2014 06:47 PM, chris wrote:
>> Hello,
>>
>> The commit 6189bc07 "inets: httpc improve pipelining" needs to be reverted.
>>
>> It introduces a crashing bug into the http client by inserting a session prior to a connection even being established, which will cause subsequent requests to crash that attempt to reuse that session.
>>
>> In httpc_manager:start_handler/2, the session must not be inserted at that point, as there may not yet, nor ever be, a valid connection.
>>
>> In the httpc_handler:handle_response(#state{status = new} = State) call chain from where the original call to insert_session was removed from httpc_handler:try_to_enable_pipeline_or_keep_alive, is actually where the session should be inserted.
>>
>>  From the test case modification in that commit, it looks like the motivation was to get rid of a sleep(4000).  But out of the entire commit, what actually caused the ability to remove the sleep from the test, was simply that every session was now marked as 'available' (renamed 'persistent' in that commit) as soon as the request received a response, if the request established a persistent connection.
>>
>> In other words, a one line code change to the original httpc_handler:try_to_enable_pipeline_or_keep_alive (renamed to httpc_handler:check_persistent and broken) could have had the same result - by always setting the 'available' session field to true for persistent connections.  Which would end up being for all stored sessions, as only persistent connections should have stored sessions anyway.  So given that, I question whether the result of such a modifictation is actually the proper thing to do, or whether the original code had its reasons for not making all persistent connections available until later in their life cycle.
>>
>> In general the entire commit was ill-conceived, and introduces other subtle bugs as well.
>>
>> Please revert the commit, as previously functional production code running for years against R16B02 and earlier releases is now crashing under R16B03(-1).
>>
>> Regards,
>>
>> Chris